-
Notifications
You must be signed in to change notification settings - Fork 187
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
CLI: allow setting options for config without profiles #5544
CLI: allow setting options for config without profiles #5544
Conversation
39651b6
to
8d94ecc
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.
Fantastic, thanks a lot @sphuber for fixing this!
I only have one main comment
|
||
This parameter type requires the command that uses it to define the ``context_class`` class attribute to be the | ||
:class:`aiida.cmdline.groups.verdi.VerdiContext` class, as that is responsible for creating the user defined object | ||
``obj`` on the context and loads the instance config. |
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.
Is there a way to validate this?
If this is straightforward (even if just indirect by checking the ctx.obj
) a check + exception might be a more effective way of enforcing this than via documentation.
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.
Can do, will add it.
The `verdi config set` command would except if the config had no configured profiles. The reason is that the command would attempt to retrieve the `Config` instance from the `ctx.obj` object. The problem is however that this is created and set on the context in the `convert` method of the `ProfileParamType`, which is used for the `--profile` option. The option specifies a default, which is the default profile, but if that is not defined, it doesn't provide anything and so the `convert` method is never called, resulting in the `ctx.obj` never being initialized and the `config` not being set. Since the config should always be present for all `verdi` comments, we should ensure that it is always loaded and set on `ctx.obj`. Therefore it is removed from `ProfileParamType.convert` and added to the constructor of a custom implementation of the `click.Context` class, called `VerdiContext`. The `VerdiCommandGroup` is updated to set the `context_class` attribute to this `VerdiContext` which will ensure that all commands will be invoked using a context that has the user defined object constructure with the config loaded and added to it. Since all commands in `verdi` will implement this class, this should guarantee that the config will always be initialized.
8d94ecc
to
3238125
Compare
Thanks for the review @ltalirz . I have added the check |
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.
Great - looks good to me!
Fixes #5543
The
verdi config set
command would except if the config had noconfigured profiles. The reason is that the command would attempt to
retrieve the
Config
instance from thectx.obj
object. The problem ishowever that this is created and set on the context in the
convert
method of the
ProfileParamType
, which is used for the--profile
option.
The option specifies a default, which is the default profile, but if
that is not defined, it doesn't provide anything and so the
convert
method is never called, resulting in the
ctx.obj
never beinginitialized and the
config
not being set.Since the config should always be present for all
verdi
comments, weshould ensure that it is always loaded and set on
ctx.obj
. Thereforeit is removed from
ProfileParamType.convert
and added to theconstructor of a custom implementation of the
click.Context
class,called
VerdiContext
. TheVerdiCommandGroup
is updated to set thecontext_class
attribute to thisVerdiContext
which will ensure thatall commands will be invoked using a context that has the user defined
object constructure with the config loaded and added to it. Since all
commands in
verdi
will implement this class, this should guaranteethat the config will always be initialized.