Migrate verdi profile to use the click infrastructure (#1591)
#1651
Conversation
Codecov Report
@@ Coverage Diff @@
## verdi #1651 +/- ##
=========================================
+ Coverage 62.2% 62.52% +0.32%
=========================================
Files 300 300
Lines 33430 33409 -21
=========================================
+ Hits 20794 20889 +95
+ Misses 12636 12520 -116
Continue to review full report at Codecov.
|
waychal
force-pushed
the
fix_1591_profile_click_cmd
branch
2 times, most recently
from
8346203 to
b073051
Compare
|
@DropD Done. |
aiida/cmdline/commands/profile.py
Outdated
| print >> sys.stderr, ("Please specify the profile to be set as the default") | ||
| sys.exit(1) | ||
| @verdi_profile.command("list") | ||
| @click.option('--no-color', is_flag=True, default=False, help='to show results without colors') |
There was a problem hiding this comment.
Please remove this option and replace all the print statements below that use color codes manually, with the click functions. click.secho('This will be in yellow: ', fg='yellow') supports setting a color out of the box. Please use that in stead
aiida/cmdline/commands/profile.py
Outdated
| echo.echo_critical(err_msg) | ||
|
|
||
| if default_profile is None: | ||
| echo.echo_error("### No default profile configured yet, run 'verdi install'! ###") |
There was a problem hiding this comment.
if you want to return out of the function use echo.echo_critical and then you can remove the return statement
aiida/cmdline/commands/profile.py
Outdated
| postgres.determine_setup() | ||
|
|
||
| import pprint | ||
| pprint.pprint(postgres.dbinfo, width=1, indent=2) |
There was a problem hiding this comment.
The idea here was to use something like pprint or json to dump the dictionary into a single string, but the printing should still be done through click.echo or one of our wrapper functions in aiida.cmdline.utils.echo
aiida/cmdline/commands/profile.py
Outdated
|
|
||
| print('Configuration folder: {}'.format(AIIDA_CONFIG_FOLDER)) | ||
| :param profiles_to_delete: list of profiles separated by space |
There was a problem hiding this comment.
Do not use explicit :param: notation in the docstring. You can define metavar=PROFILES in the click.argument and use that name in the docstring e.g.:
Delete PROFILES from the aiida configuration file along with
their associated database and repository.
There was a problem hiding this comment.
This is because in click, the docstring of a command becomes it's help text
waychal
force-pushed
the
fix_1591_profile_click_cmd
branch
2 times, most recently
from
46cd44c to
2f158c9
Compare
waychal
force-pushed
the
fix_1591_profile_click_cmd
branch
from
2f158c9 to
f1e5476
Compare
| postgres.determine_setup() | ||
|
|
||
| import json | ||
| echo.echo(json.dumps(postgres.dbinfo, indent=4)) |
There was a problem hiding this comment.
This debug echo statement should be removed before merging
There was a problem hiding this comment.
It is not a debug statement and printed to give information to user.
There was a problem hiding this comment.
Then I feel it requires some explanation, because this will simply print the dictionary, and the user will have no idea what it means.
There was a problem hiding this comment.
here we are telling user which database will be deleted. I can print dbinfo in key: value pair. Do you have any suggestions?
|
I am less concerned about the format. The output must contain an
explanation.
Ok:
```
$ Verdi profile delete
... # unrelated output
The following database will be deleted:
Name: ...
Host:. ..
...
```
Not ok:
```
$ Verdi profile delete
... # unrelated output
Name: ...
Host:. ..
...
```
…On Tue, Jul 10, 2018, 17:46 Snehal Kumbhar ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In aiida/cmdline/commands/profile.py
<#1651 (comment)>:
> - .format(profile_to_delete)):
- print("Deleting configuration for profile '{}'.".format(profile_to_delete))
- del profiles[profile_to_delete]
- update_config(confs)
+ profile = profiles[profile_to_delete]
+ except KeyError:
+ echo.echo_info("Profile '{}' does not exist".format(profile_to_delete))
+ continue
+
+ postgres = Postgres(port=profile.get('AIIDADB_PORT'), interactive=True, quiet=False)
+ postgres.dbinfo["user"] = profile.get('AIIDADB_USER')
+ postgres.dbinfo["host"] = profile.get('AIIDADB_HOST')
+ postgres.determine_setup()
+
+ import json
+ echo.echo(json.dumps(postgres.dbinfo, indent=4))
here we are telling user which database will be deleted. I can print
dbinfo in key: value pair. Do you have any suggestions?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#1651 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABMqBIwW05KPMjNcAdWXwl9z_S05I0Btks5uFMxWgaJpZM4Ur5K7>
.
|
No description provided.