Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rfc7: break long test expressions to one per line #437

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Dec 2, 2024

Problem: We've adopted a defacto guideline that long test expressions connected by "AND" or "OR" operators should be broken to one test expression per line. This is not documented.

Add this to RFC 7.

@chu11 chu11 force-pushed the rfc7_c_test_expressions branch from 35f349e to 3b06745 Compare December 2, 2024 22:42
Copy link
Member

@cmoussa1 cmoussa1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except does example need to be fixed or maybe de-tabbed?

spec_7.rst Outdated
Comment on lines 96 to 97
&& (out->bytes > MIN_BYTES
&& out->bytes <= MAX_BYTES)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

er that doesn't appear to be at the same indent level?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note the parens, its style is if (A && (B &&C)) leading to the longer version

if (long-test-1
    && (long-test-2
           && long-test-3)) {
}

i added it to demonstrate that a long expression within an expression should also be broken up.

Copy link
Member

@garlick garlick Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I misread that.

However, do we want that? I would not have put a line break on the second (compound) expression.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, lets tweak for this example. I changed it to

+    if (out->type == MY_EXPECTED_TYPE
+        && out->bytes > LOW_END_RANGE
+        && out->bytes < HIGH_END_RANGE) {
+        ...
+    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Controversy avoided :-)

Problem: We've adopted a defacto guideline that long test expressions
connected by "AND" or "OR" operators should be broken to one test
expression per line.  This is not documented.

Add this to RFC 7.
@chu11 chu11 force-pushed the rfc7_c_test_expressions branch from 3b06745 to 11aa0bc Compare December 2, 2024 23:37
@chu11
Copy link
Member Author

chu11 commented Dec 2, 2024

re-pushed, tweaking the example per conversation above

@mergify mergify bot merged commit 33eedc0 into flux-framework:master Dec 3, 2024
7 checks passed
@chu11 chu11 deleted the rfc7_c_test_expressions branch December 3, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants