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

Remove incorrect development rule about alphabetizing code #11437

Conversation

findepi
Copy link
Member

@findepi findepi commented Mar 11, 2022

Documentation listings, like list of connectors, should probably be
alphabetized, but code sections do not. The logic ordering of
components, and grouping of related things, is more important.

Documentation listings, like list of connectors, should probably be
alphabetized, but code sections do not. The logic ordering of
components, and grouping of related things, is more important.
@cla-bot cla-bot bot added the cla-signed label Mar 11, 2022
@losipiuk losipiuk requested a review from kokosing March 11, 2022 17:25
Copy link
Member

@leveyja leveyja left a comment

Choose a reason for hiding this comment

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

I think we should work on this with purpose, not just fire off a PR to remove documentation.

(also, alphabetizing is good for diffs/merges - otherwise everyone attempts to add a line at the "end" of an enum, etc, => Merge confilcts. So it is a good thing in certain cases.)

Also, I'd like this mis-information to be tackled in the same commit:

The code style template for the project can be found in the codestyle repository along with our general programming and Java guidelines.

The code style template for the project can indeed be found in the codestyle repository, https://github.com/airlift/codestyle - however "our general programming and Java guidelines" cannot (unless we mean general programming "code style" guidelines and Java "code style" guidelines).

I'm happy to draft a PR + work with you @findepi (and all other reviewers) to discuss the overall changes before we get into reviews 👍

@leveyja leveyja self-requested a review March 11, 2022 18:39
Copy link
Member

@leveyja leveyja left a comment

Choose a reason for hiding this comment

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

I think we should work on this with purpose, not just fire off a PR to remove documentation.

(also, alphabetizing is good for diffs/merges - otherwise everyone attempts to add a line at the "end" of an enum, etc, => Merge confilcts. So it is a good thing in certain cases.)

Also, I'd like this mis-information to be tackled in the same commit:

The code style template for the project can be found in the codestyle repository along with our general programming and Java guidelines.

The code style template for the project can indeed be found in the codestyle repository, https://github.com/airlift/codestyle - however "our general programming and Java guidelines" cannot (unless we mean general programming "code style" guidelines and Java "code style" guidelines).

I'm happy to draft a PR + work with you @findepi (and all other reviewers) to discuss the overall changes before we get into reviews 👍

@martint
Copy link
Member

martint commented Mar 11, 2022

"our general programming and Java guidelines" cannot

Aspirationally, that where we intended to capture those guidelines. We never got around to doing that, so that's why it seems like the comment is untrue. The fix would not be to remove the line, but to start capturing those guidelines there.

@findepi
Copy link
Member Author

findepi commented Mar 11, 2022

i don't see many places where we strive to alphabetize, so it is a non-rule apparently.

i see many places where we shouldn't try to alphabetize, because other, more important, logical ordering should be applied.

that's why i think the sentence should be removed.

@findepi findepi closed this Mar 11, 2022
@findepi findepi deleted the findepi/remove-incorrect-rule-about-alphabetizing-code-53fcee branch March 11, 2022 21:49
@findepi findepi restored the findepi/remove-incorrect-rule-about-alphabetizing-code-53fcee branch March 14, 2022 16:59
@findepi findepi reopened this Mar 14, 2022
@findepi findepi merged commit 90acb84 into trinodb:master Mar 14, 2022
@findepi findepi deleted the findepi/remove-incorrect-rule-about-alphabetizing-code-53fcee branch March 14, 2022 21:08
@github-actions github-actions bot added this to the 374 milestone Mar 15, 2022
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

4 participants