Deprecate methods that refer to a computer's label as name #4309
Conversation
sphuber
force-pushed
the
fix/4304/computer-deprecated-methods
branch
from
795c49c
to
269bc7d
Compare
All entities use `label` as the human readable string identifier, but `Computer` was using `name`. This was already changed in the front-end ORM in a previous commit where a `label` property was introduced and the old `name` properties were deprecated, however, a few derivative methods in other classes were missed and still use contain "name". These methods are now also deprecated: * `verdi computer rename`: use `verdi computer relabel` instead * `Code.get_computer_name`: use `self.computer.label` instead * `Code.get_full_text_info`: will be removed * `RemoteData.get_computer_name`: use `self.computer.label` instead * `Transport.get_valid_transports`: `get_entry_point_names` instead Finally, deprecations of `Computer` getters and setters as introduced in commit 592dd36 were still being used internally leading to a lot of deprecation warnings. These have now been properly replaced.
sphuber
force-pushed
the
fix/4304/computer-deprecated-methods
branch
from
269bc7d
to
3480c48
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4309 +/- ##
===========================================
- Coverage 79.22% 79.04% -0.17%
===========================================
Files 468 468
Lines 34556 34602 +46
===========================================
- Hits 27373 27348 -25
- Misses 7183 7254 +71
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
|
||
|
|
||
| @verdi_computer.command('rename') | ||
| @arguments.COMPUTER() | ||
| @arguments.LABEL('NEW_NAME') | ||
| @deprecated_command("This command has been deprecated. Please use 'verdi computer relabel' instead.") |
There was a problem hiding this comment.
Completely unrelated, but should we maybe have @deprecated_command automatically append the "This command has been deprecated" text to the string passed through, so it is not necessary to include that part each time?
There was a problem hiding this comment.
That would certainly be an option, although low priority I would say. Usually you copy this line from some other place anyway so it is already there.
| @@ -168,7 +168,36 @@ def code_duplicate(ctx, code, non_interactive, **kwargs): | |||
| @with_dbenv() | |||
| def show(code, verbose): | |||
| """Display detailed information for a code.""" | |||
| click.echo(tabulate.tabulate(code.get_full_text_info(verbose))) | |||
There was a problem hiding this comment.
So, can I ask what is the intended purpose of the commands show and computer_show modified in this PR? Because I'm not sure I understand why you are removing the general get_full_text_info method to implement two relatively similar versions directly into each of those commands.
There was a problem hiding this comment.
Note that one is verdi code show and the other verdi computer show. I removed the methods from the classes because they don't really belong there. Especially the Computer one was bad since it was baking in the formatting. It was returning a string fully formatted. This is really inflexible as the caller cannot do anything to modify it. The Code one is already slightly better since it simply returned a list of tuples. Still I think deciding what information should be displayed is still very situation dependent and so there is no need to have a dedicated method for this. It was no wonder that the methods were also just used in one place, namely the show CLI commands. Might as well put that logic there so it has all the freedom it needs to choose what and how to display the information
There was a problem hiding this comment.
Ah, I see, I didn't notice these were methods of different classes. Thanks!
Fixes #4304 and fixes #4313
All entities use
labelas the human readable string identifier, butComputerwas usingname. This was already changed in the front-endORM in a previous commit where a
labelproperty was introduced andthe old
nameproperties were deprecated, however, a few derivativemethods in other classes were missed and still use contain "name".
These methods are now also deprecated:
verdi computer rename: useverdi computer relabelinsteadCode.get_computer_name: useself.computer.labelinsteadCode.get_full_text_info: will be removedRemoteData.get_computer_name: useself.computer.labelinsteadTransport.get_valid_transports:get_entry_point_namesinsteadFinally, deprecations of
Computergetters and setters as introducedin commit 592dd36 were still being used internally leading to
a lot of deprecation warnings. These have now been properly replaced.