-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
refactor(aliases): sort #1743
refactor(aliases): sort #1743
Conversation
georgettica
commented
Dec 16, 2020
- move all comments to the same line so they wont be jumbled
related to #1732 |
83d4745
to
eb5d222
Compare
now it's partially related to #1696 |
Greetings! A straight sort is useful, but I'm inclined to think that grouping by sub-command or maybe related actions has value too:
The alias file serves as its own documentation, as there's (currently) no other way to know what aliases an alias file introduces, nor to get detailed info on a specific alias, other than to look inside the alias file itself. To that end, I think we should have some kind of goal for alias files to read well. |
It was just very hard to sort with the comments sparsed that way. Or if you have a good sorting technique that keeps comments bear them I would be glad to hear about it |
Hi again,
My comment wasn't related to the idea of moving alias doc-comments to a single line - It was related the idea of blindly sorting sorting the aliases in general. (I understand that, when moving these around en-mass, moving the alias comments to the same line makes it easier to keep them with their associated alias) I think that, if we're going to re-arrange the aliases, we should have a better goal than just plainly sorting them. Organizing them by some common feature/function makes more sense to me. THIS is what I referring to in my comment. Additionally, I think you may have added too many changes to this PR, ie changes to the following files:
These changes don't really ave anything to do with the They seem useful, but they should be part of a separate, stand-alone PR and not part of this one. I recommend moving them into a separate, new PR so we can discuss them (and possibly merge them) regardless of what happens here in this PR. -DF |
@georgettica I agree with @davidpfarrell Move the docker commit to a separate PR and remove it and the auto-reload one from this PR, and then we can continue with this one. Great job by the way! Keep up the good work 😄 |
That sounds great! |
e5234f5
to
4a9e342
Compare
diverged dockerfile and linting into #1753 |
nice work @georgettica! I think that what you did with the Lemme know what you think! |
I am fine with that :) |
any updates @georgettica ? |
You want more sections? Or is that enough |
I want to do this all in this PR, so we can make sure the file will stay sorted after your PR. If you can finish this up with sections as I said it would be amazing! |
Now that I know, I'll do that |
0534601
to
c3a01db
Compare
hey @georgettica, I updated your PR with a rebase on master and some changes, you are welcome to look at my addition (c3a01db) and see if you like it @cornfeedhobo @davidpfarrell @nwinkler @tbhaxor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NoahGorny fixing as you see fit, it really pushes this PR to conclusion I think.
For me it looks ok (though I can't approve)
@cornfeedhobo wanna take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new organization strategy looks good!
I spent some time comparing the old and new versions of the file to confirm that every alias was covered. Everything appears to be there.
Best I can tell, this is good to go - thanks for the effort !
As a side note : We may need to adopt a more nuanced policy regarding quotes used for alias definitions ... The |
- move all comments to the same line so they wont be jumbled - also add to clean_files.txt
805477a
to
dd4e410
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased again, and merging as @davidpfarrell approved.
Thank you guys for the effort you put in @georgettica @davidpfarrell ! You rock!
Thanks for helping aswell @NoahGorny |