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.

Sign up for GitHub

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

Jump to bottom

Add --config option to use different config file. #79

Closed
wants to merge 5 commits into from
Closed

Add --config option to use different config file. #79

wants to merge 5 commits into from

Conversation

lucc
Copy link
Owner

@lucc lucc commented Jul 19, 2016

No description provided.

@scheibler
Copy link
Collaborator

Nice idea. But for me it doesn't work. Problem is, that the set_default_subparser function must be
called before parse_args. The set_default_subparser function requires the default action which must
come from the config file. But loading of that config file is not possible cause we don't get the
path of the new config file before parse_args (lines 1459 to 1460 of khard.py).

The only thing, I could imagine is to take the default action from the default config file, parse
args, get path to new config file and recreate config object. Or do I miss something here?

@lucc lucc mentioned this pull request Jul 22, 2016
Closed
@lucc
Copy link
Owner Author

lucc commented Jul 22, 2016

That is right. My solution would be #81, alas to remove the set_default_subparser. This can only be done now because only python3 can detect a missing subcommand like it is done in #81.

You should merge #81 before this one.

@scheibler
Copy link
Collaborator

Sorry, that doesn't work.

First, as you've already pointed out, command line options wouldn't work any longer for the default
action. Unfortunately this also includes the indispensable search option. Example: My default action
is phone, cause I want to look up phone numbers with minimal typing like khard first_name last_name. But that would not be possible any longer. The only thing I still could do with the
default action is show all phone numbers in my address book. But then the default action would be
almost useless (in my opinion).

Second: Try to set phone or email as your default action and run khard without any action or command
line option. Then you will get an AttributeError at line 1608. It seems, that additionally added
command line options like --parsable are not respected in that case.

Please also see the commit a67f88a for further information.

@lucc
Copy link
Owner Author

lucc commented Sep 2, 2016

I have another idea how we could do this: We could do runs of parse_args() and construct some command line parsers intelligently to incorporate the default action in the second run. But I will wait for the other open PRs to make merge conflicts less likely. Especially I would like you to merge #94 before I revisit this.

@lucc lucc mentioned this pull request Sep 18, 2016
Closed
@lucc lucc changed the base branch from master to develop September 20, 2016 22:55
@lucc
Add a --config option to use an alternate config file
b2c8273 
The argparse patch is also removed.

The command line is parsed in two steps in order to make it possible to

- use the config file given on the command line
- display the help message before parsing the config file
- set the debug option before parsing the config file
- get the default command from the config file if none was given on the
  command line

Currently there is one undesired side effect:  If no subcommand is given on
the command line no options for the subcommand can be given on the command
line as well.  Previously this was possible.
@lucc
Split main() function
795e31f
@lucc
Add docstrings to actions.Actions' methods
f9b701f
@lucc
Parse the command line remainder instead of reparsing
39c643c 
On the second parsing step the remainder of the first parsing step is parsed
instead of reparsing the whole command line.  This makes it possible to inject
the default command from the config file with minimal effort if no subcommand
was given on the command line.

This fixes the regression mentioned in b2c8273: It is now again possible to
specify arguments to the default command without specifying the default
command itself (with the exception of all options that are valid to the main
parser).

This also makes it possible to specify general options (like --debug or
--config) after the subcommand or its arguments.  They will be processed by
the first parsing run.
@lucc
Copy link
Owner Author

lucc commented Sep 20, 2016 •
edited

I implemented the two step parsing and changed the merge target to develop. It

  • adds a --config option
  • removes the old argparse patch (so Remove argparse patch #81 is obsolete after this)
  • processes --help before the config file is read (config errors wont stop the help text from being displayed)
  • reads the default command from the correct config file (even wth --config)
  • arguments to the default command can be given on the command line without giving the default command on the command line
  • feature/bug? global options can be given after the subcommand, subcommand options can not be given before the subcommand (the parser will think no subcommand was specified and use the default one, the option will probably be used as a search term then, depending on the subcommand)

If you merge this try to keep these commits as one of the commit messages mentiones a commit hash of another (or you have to change the commit message).

@scheibler
Copy link
Collaborator

Looks really good. Even the default action is processed well. Thanks for reorganizing.

I only found one bug so far. If you run

khard -h

the general help text is shown but if you run

khard phone -h

it's also the general help and not the help text for the phone action.

@lucc
Fix help option of subcommands
ed50375 
The remainder of the command line is explicitly collected in the first parsing
step.  This allows the parse_args() method to be used instead of
parse_known_args().  The latter did skip any unknown args in order to process
more elements of sys.argv.  Thereby it did find the help options of
subcommands.  With the remainder argument in the first parser parse_args() can
be forced to stop processing at the first unknown argument.

This has the side effect that no global options can be given after the first
non global argument.  This is the case with either an explicit or
implicit/default subcommand.  So `khard some-search-terms -h` will give the
help of the default subcommand.
@lucc
Copy link
Owner Author

lucc commented Sep 22, 2016

I think I fixed it but there is a small (in my opinion acceptable) trade off: Some commits back we could specify global options after the subcommand. Now this can't work anymore b/c it was exactly that which "eat" the -h of the subcommand.

@scheibler
Copy link
Collaborator

scheibler commented Sep 22, 2016 via email

@lucc
Copy link
Owner Author

lucc commented Sep 22, 2016

I already had tests for this in #96 and didn't notice :(

@scheibler
Copy link
Collaborator

Merged into develop branch.

@lucc
Copy link
Owner Author

lucc commented Sep 24, 2016 •
edited

How do you merge these commits all the time to get new commits? (Git does not recognize that you merged them. Did you dogit checkout develop; git merge config-option?)

Thanks anyway

@lucc lucc closed this Sep 24, 2016
@lucc lucc deleted the config-option branch September 24, 2016 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lucc @scheibler