-
Notifications
You must be signed in to change notification settings - Fork 284
Hide withdraw in golecli account if not mainnet #3684
Hide withdraw in golecli account if not mainnet #3684
Conversation
golem/interface/cli.py
Outdated
@@ -40,6 +41,19 @@ def exit(self, status=0, message=None): | |||
raise ParsingException(message, self) | |||
|
|||
|
|||
def adapt_children(children: Dict[Text, Callable]) -> Dict[Text, Callable]: |
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.
@dybi ... on the surface, this function seems generic but really it's only applicable to a very specific case and hardly reusable... wouldn't it make sense to just get rid of that key specifically in the _build_parser
without having a separate function for this task?
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.
@shadeofblue, you've got a point - the name of the function is too generic. However, I would be for having it separated from the rest of the code for the sake of better modularity and unit-testing.
…thdraw` and change dict comprehension to deepcopy
18b31ec
to
b357060
Compare
…w-in-golecli-account-if-not-mainnet
…rmining if `withdraw` should be available in golemcli
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.
looks good as far as I can tell.
@Krigpl , could you confirm?
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.
IMHO it would be nice to explicitly disable a method in golem.interface.client.account.Account.withdraw()
. Possibly by adding a modifier to @command
decorator.
Codecov Report
@@ Coverage Diff @@
## develop #3684 +/- ##
===========================================
+ Coverage 89.11% 89.13% +0.02%
===========================================
Files 202 202
Lines 18064 18084 +20
===========================================
+ Hits 16097 16120 +23
+ Misses 1967 1964 -3 |
@jiivan the problem with the proposed approach is that |
fcf9515
to
65478e0
Compare
golem/interface/command.py
Outdated
if m.enabled: # type: ignore | ||
filtered.append((name, m)) | ||
else: | ||
filtered.append((name, m)) |
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.
if getattr(m, 'enabled', False):
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.
return [(name, m) for name, m in methods if getattr(m, 'enabled', False)]
I'm not sure if this list comprehension is worthy of a separate function. I would just use it in CommandHelper.get_methods()
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.
it is no longer there - as stated above:
@group
and@command
decorators are evaluated at import and whether we are running on miannet or not is determined later (after arguments are parsed)
so it seems not to work properly: at the time command_enabled
is evaluated IS_MAINET
flag is still False
- it gets to True
later (if -m
is provided) when arguments are parsed
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.
Yeah, CommandHelper
needs major refactoring, but it's not top priority right now.
resolves: https://github.com/golemfactory/golem/issues/3012