refactor: Minor refactoring of 'deprecate_args.py'. #2800
Conversation
prmukherj
requested review from
mkundu1,
seanpearsonuk,
ajain-work,
dnwillia-work,
hpohekar,
h-krishnan and
raph-luc
as code owners
| @@ -19,12 +19,12 @@ def wrapper(*args, **kwargs): | |||
| argument.""" | |||
| if old_arg in kwargs: | |||
| warnings.warn( | |||
| f"'{old_arg}' is deprecated. Use '{new_arg}' instead.", | |||
| f"'{old_arg}' is deprecated. Using '{new_arg}' instead.", | |||
There was a problem hiding this comment.
@raph-luc, I think using the "new_arg = new_value" here will not make much sense, because the final value will depend on both the returned value of converter (which can be None) as well as the set value of new_arg (if already set along with the old arg). What do you think?
There was a problem hiding this comment.
@prmukherj If user hasn't passed new_arg and is told that the code is using new_arg then that is confusing (what the code is actually likely to be doing is using old_arg via transformation, which is too much information to provide). The only necessity here is to issue guidance, which we are doing in the current version.
There was a problem hiding this comment.
Thanks @prmukherj, can't we figure out what the actual new_value is going to be used, and report that so the user knows what exactly to specify next time? For example, I was thinking the warning would ideally show something like: 'show_gui = True' is deprecated. Using 'ui_mode = UIMode.GUI' instead. (which is what the code is doing, right?)
@seanpearsonuk: in my understanding this deprecate_argument isn't just providing guidance, the code is using the new argument instead of the old one, which requires different options (though the result should be the same), and I think we can let the user know what exactly is being done so they don't have to look up this new argument next time to figure out how to use it. Does that make sense?
There was a problem hiding this comment.
At this point, the code has only established that the old argument has been passed in -- its value has not yet been analysed. What you're proposing would require some re-work.
There was a problem hiding this comment.
This comment is on line 22, the value is analyzed on lines 25-26 and then set on line 27, correct? If this makes sense, I am proposing we move this warning to after the new argument and value are set, and show to the user what exactly was done. This is so the user will know what non-deprecated argument and value to use for the next time they use PyFluent.
This is a minor re-work if I'm not missing anything.
Edit: we may need to verify if the string representation of the UIMode enum would work properly when printing it like this
After the earlier PR was merged I marked some comments from @raph-luc, which made sense. This PR addresses those minor issues.