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
Shorter config process #127
Conversation
Add --sections flag to app.go according to T657, parse them into a string array (check for invalid arguments and abort) and pass them to Configure(). For now Configure() doesn't do anything with them yet.
Use the split argument list (slice) just for validation purposes as it's substantially easier to do `.contains` in a string instead of a slice. As such, pass the `configSections` arguments to `Configure()` and check the existence of each one before showing the options to the user. An empty argument list is replaced by "server db app" so everything is there negating the need to check anything else in `Configure()`. In the same vein the default is "server db app". The parsing is done in `app.go` alongside the other flags instead of `main.go` as described in T657.
Btw, currently the program just ignores |
Move flag parsing to main.go as per the issue description
That might be a good idea, error with the usage output. |
Ok will do. |
Now that I look at it... As the code is now the whole flag parsing process is to match the first flag found and then ignore anything else, so it seems a bit out of place to throw an error in this particular combination (or lack thereof). It's inconsistent to not throw any error at |
One approach might be to use |
I like this, since it will not fail silently in either case, without having to rework the whole thing. Let me take a stab at it. |
For some reason my earlier response didn't send, but I'd say yeah, it should fail silently as it does with other flags. The command-line side of WF is used infrequently enough that I'd say it's not worth getting too far in the weeds with it. |
Ok then, if there aren't any other changes needed you can merge the PR as is. |
This runs `go fmt` on changed files and moves around some blank lines.
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.
Just two issues so far:
- Doesn't look like some files had
go fmt
run on them. We all want to be sure to do that before committing, so everyone's code is consistent. As a helpful hint, most people set up their editor to run that automatically every time they save the file. (I went ahead and fixed this) - The "Environment" prompt is now outside the Server section. It should stay inside that section, and not show up when a user tries to configure sections without
server
Went ahead and fixed these up. Thanks @qwazix! Merging now. |
Thanks, I missed that |
No worries! |
This resolves T657: shorter-config-process by adding a --sections parameter that takes a space separated string of sections (app, db and server are the current valid ones).