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

Shorter config process #127

Merged
merged 8 commits into from Jul 1, 2019
Merged

Shorter config process #127

merged 8 commits into from Jul 1, 2019

Conversation

qwazix
Copy link
Collaborator

@qwazix qwazix commented Jun 21, 2019

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).

Michael Demetriou added 5 commits June 11, 2019 20:18
This solves the error 500 on the /api/me endpoint.

Replace token search query `=` with `LIKE` to fix sqlite complaining about
no valid tokens. Also checked with MySQL and it still works after the change.
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.
This solves the error 500 on the /api/me endpoint.

Replace token search query `=` with `LIKE` to fix sqlite complaining about
no valid tokens. Also checked with MySQL and it still works after the change.
@qwazix
Copy link
Collaborator Author

qwazix commented Jun 21, 2019

Btw, currently the program just ignores --sections if --config is not present. Do we want to throw an error?

Move flag parsing to main.go as per the issue description
@ghost
Copy link

ghost commented Jun 27, 2019

That might be a good idea, error with the usage output.

@qwazix
Copy link
Collaborator Author

qwazix commented Jun 27, 2019

Ok will do.

@qwazix
Copy link
Collaborator Author

qwazix commented Jun 27, 2019

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 writefreely --config --gen-keys and do so at writefreely --gen-keys --sections app even though both exhibit the same behavior: they ignore the second flag.

@joyeusenoelle
Copy link
Contributor

One approach might be to use flag.Visit to count the number of supplied flags; if it's >1 and the flags aren't --config and --sections, print an error ("One at a time, please") and then continue with the first flag as normal.

@qwazix
Copy link
Collaborator Author

qwazix commented Jun 27, 2019

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.

@thebaer
Copy link
Member

thebaer commented Jun 27, 2019

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.

@qwazix
Copy link
Collaborator Author

qwazix commented Jun 28, 2019

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.
Copy link
Member

@thebaer thebaer left a 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

@thebaer thebaer added this to the 0.10 milestone Jul 1, 2019
@thebaer
Copy link
Member

thebaer commented Jul 1, 2019

Went ahead and fixed these up. Thanks @qwazix! Merging now.

@thebaer thebaer merged commit 18bafad into develop Jul 1, 2019
@thebaer thebaer deleted the shorter-config-process branch July 1, 2019 18:15
@qwazix
Copy link
Collaborator Author

qwazix commented Jul 1, 2019

Thanks, I missed that go fmt, though I admit I did read about it in the style guide :-/ Will make sure to run it next time.

@thebaer
Copy link
Member

thebaer commented Jul 1, 2019

No worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants