refactor(cli): centralize options#2767
Conversation
|
This looks great! |
| ); | ||
| } | ||
|
|
||
| function indent(str, spaces) { |
There was a problem hiding this comment.
Heh, theoretically you could use prettier to format the help 😉
|
Probably something for a future PR, but I've been thinking about generating the options part of README.md and options.md (which is probably out of date) from code. We could then support |
|
Something like markdown-magic? |
|
So long as we use the prettier plugin 👍 |
| const optionArray = Object.keys(options).reduce( | ||
| (current, name) => current.concat(options[name]), | ||
| [] | ||
| ); |
There was a problem hiding this comment.
Isn't this better written as Object.keys(options).map(name => options[name])? Or am I missing something?
There was a problem hiding this comment.
Oops, forgot it's an array too. 😅
|
I have another idea to further reduce duplicate code in this PR, mark it as WIP now. |
ikatyang
changed the title
- Move Format options first, since they feel the most important. - Move --find-config-path to the Config category. - Merge Command into Other. It wasn't clear what "Command" options are, and the Other category isn't too long anyway. - Move --range-end and --range-start into Other. The aren't Format options.
- Move the very important options --write and --list-different to the top, in the new Output category. It could be argued that --help and --version also belong there, but I don't think they deserve such a prominent position, so they are left in the Other category. --find-config-path could also be considered an Output option, but I think it fits better in the Config category. - Now that there's an Output category, should there also be an Input category? I thought about that, but the only candidates for such a category are --stdin and --stdin-filepath and I don't think they are important enough to have their own category, so they stay in Other. - Move --cursor-offset, --range-end and --range-start to a new Editor category, since they are mainly for editor integrations. - Document how Prettier deals with input and output by default.
| firstCategories.indexOf(category) === -1 && | ||
| lastCategories.indexOf(category) === -1 | ||
| ); | ||
| const allCategories = firstCategories.concat(restCategories, lastCategories); |
There was a problem hiding this comment.
Do we really need this block? Can't we just do:
const categoryOrder = [
constant.CATEGORY_OUTPUT,
constant.CATEGORY_FORMAT,
constant.CATEGORY_CONFIG,
constant.CATEGORY_EDITOR,
constant.CATEGORY_OTHER
];in cli-constant.js and be done with it? What do you think, @ikatyang?
There was a problem hiding this comment.
My original thought is that we might forget to edit the ordering in cli-util.js, cause unordered category missing thus the snapshot tests passed with no change. We have code review to catch that kind of things, but I'm not sure if we will remember to 😅. Any thoughts?
There was a problem hiding this comment.
Yeah, I guess it makes sense to keep this to help us from making mistakes in the future. It's not so many lines after all.
|
Let's merge this after 1.6.2 has been released. Just in case. |
|
Based on discussion in #2804 (comment), since we are going to make the 1.6.2 release 1.7.0 instead, we are going to merge this |
|
@suchipi could you use |
|
Will do. I was going to do rebase and merge, but this one said it had merge conflicts (if I did rebase), and so I had to make a merge commit. But I didn't try squash and merge. I'll make that my default for this repo in the future 👍 |
|
@vjeux should we disable the other two options so others don't get caught by this? |
lock
Bot
added
the
locked-due-to-inactivity
5 participants
main:
side:
e/errtoerrorresolveConfigtologResolvedConfigPathOrDie