search

aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
412 stars 184 forks source link

CLI: verdi profile setdefault to set-default #6426

Closed agoscinski closed 3 weeks ago

agoscinski commented 1 month ago

Fixes #2910

Since verdi user uses set-default, but verdi profile uses setdefault, we make the two consistent by using set-default for profile. Old command is marked as deprecated till version 3

sphuber commented 3 weeks ago

@agoscinski can we wrap this one up for the release?

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 73.20%. Comparing base (ef60b66) to head (1ab92bf). Report is 35 commits behind head on main.

Files Patch % Lines
src/aiida/cmdline/commands/cmd_profile.py 62.50% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6426 +/- ## ========================================== - Coverage 77.51% 73.20% -4.31% ========================================== Files 560 562 +2 Lines 41444 41808 +364 ========================================== - Hits 32120 30600 -1520 - Misses 9324 11208 +1884 ``` | [Flag](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6426/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | Coverage Δ | | |---|---|---| | [presto](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6426/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | `73.20% <62.50%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

agoscinski commented 3 weeks ago

Okay rebased, tests should pass now. Separated this PR into two commits, since the versioning has become a separated feature.

sphuber commented 3 weeks ago

Okay rebased, tests should pass now. Separated this PR into two commits, since the versioning has become a separated feature.

Thanks @agoscinski but I would prefer to get rid of the first commit. With your previous PR, the deprecated_command decorator is no longer needed. I just opened a PR to deprecate it and update all deprecated commands to use your new method: https://github.com/aiidateam/aiida-core/pull/6461

So it doesn't make sense to add this new feature to something we are deprecating and no longer using.

agoscinski commented 3 weeks ago

Sure! Makes sense. Just dropped it

sphuber commented 3 weeks ago

Thanks a lot @agoscinski . As an aside, it is not necessary to add the PR number in the commit message title. This is done automatically when merging through github. Also, we don't really add issue numbers to commit messages. We just add them to the OP of the PR instead. That should be sufficient