Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: more detailed deprecated argument warning message #2808

Merged
merged 10 commits into from
May 22, 2024

Conversation

raph-luc
Copy link
Member

@raph-luc raph-luc commented May 11, 2024

I believe users will appreciate a more helpful deprecation message for arguments that we mark as deprecated.

The same logic should be applicable for any arguments that we deprecate and are replaced by newer arguments. If time allows, please help identify any gaps/issues.

Also updated the warning stack level so that it refers to the call that results in the deprecation warning, rather than the source of the deprecation warning in the code (see #2808 (comment) below)

Examples for launch_fluent which is the only method using deprecate_argument at the moment

Before the changes in this PR:

  • Any launch_fluent(show_gui=...) specification
    PyFluentDeprecationWarning: 'show_gui' is deprecated. Use 'ui_mode' instead.

After the changes in this PR:

  • launch_fluent(show_gui=True):
PyFluentDeprecationWarning: 'show_gui' is deprecated. Use 'ui_mode' instead.
pyfluent.general WARNING: Using 'ui_mode = "gui"' for 'launch_fluent()' instead of 'show_gui = True'.
  • launch_fluent(show_gui=False):
PyFluentDeprecationWarning: 'show_gui' is deprecated. Use 'ui_mode' instead.
pyfluent.general WARNING: Using 'ui_mode = None' for 'launch_fluent()' instead of 'show_gui = False'.
  • launch_fluent(show_gui=None):
PyFluentDeprecationWarning: 'show_gui' is deprecated. Use 'ui_mode' instead.
pyfluent.general WARNING: Using 'ui_mode = None' for 'launch_fluent()' instead of 'show_gui = None'.
  • Both specified e.g. launch_fluent(show_gui=True, ui_mode="gui"): show_gui is ignored and only ui_mode applies (no change of behavior in this PR):
PyFluentDeprecationWarning: 'show_gui' is deprecated. Use 'ui_mode' instead.
pyfluent.general WARNING: Ignoring 'show_gui = True' specification for 'launch_fluent()', only 'ui_mode = "gui"' applies.

For previous discussion see #2800 (comment)

@raph-luc raph-luc linked an issue May 11, 2024 that may be closed by this pull request
@mkundu1
Copy link
Contributor

mkundu1 commented May 13, 2024

I think what we have here is a combination of a warning and an information. We shall warn user that a deprecated arg-name is entered and what is to use instead. I think the message 'show_gui' is deprecated. Use 'ui_mode = "gui"' instead of 'show_gui = True' is somewhat incomplete as we are not telling users what to do for 'show_gui = False'. The previous message 'show_gui' is deprecated. Use 'ui_mode' instead. is better as in that case user will probably look-up the usage of ui_mode in the documentation and will get the full picture.

We shall also inform the user about how the value is converted for the current input, otherwise user may think that the entered value has been discarded and try to re-execute the function again.

So, perhaps we can structure the code in this way:

  1. Show warning for the deprecated arg-name usage.
  2. Convert deprecated arg-name/value to new arg-name/value.
  3. Show information about the above conversion.

@raph-luc
Copy link
Member Author

raph-luc commented May 14, 2024

So, perhaps we can structure the code in this way:

  1. Show warning for the deprecated arg-name usage.
  2. Convert deprecated arg-name/value to new arg-name/value.
  3. Show information about the above conversion.

@mkundu1 do you mean just using a warning print and a separate information print (I imagine you mean something like logging.info()) instead of putting it all together in a single warning message as I suggest here? That is closer to what I was originally suggesting as well (giving the user all the information in a more detailed way), but @seanpearsonuk raised a point that it might be too much information to provide: #2800 (comment) which is a fair point and that is partly why I suggest we be less verbose and more direct as in this PR.

Regardless, this PR already addresses all 3 points in my understanding, for point 1 we show a warning for the deprecated argument: 'show_gui' is deprecated.; point 2 is already handled by the converter function that @prmukherj had included and does the exact conversion from the previous argument to the new one, that is what we are using here; point 3 is the second part of the warning: Use 'ui_mode = "gui"' instead of 'show_gui = True'. which shows the exact conversion performed and what the user should use next time.

Let me know if you want to meet offline to discuss this further

Edit: addressed by 52e6a8e

@raph-luc raph-luc marked this pull request as draft May 21, 2024 20:20
@raph-luc raph-luc marked this pull request as ready for review May 21, 2024 20:31
@raph-luc raph-luc force-pushed the maint/more-details-deprecate-warn branch from 6835c63 to 8f3d872 Compare May 22, 2024 15:33
@raph-luc raph-luc enabled auto-merge (squash) May 22, 2024 16:00
@raph-luc raph-luc merged commit 93d66ad into main May 22, 2024
26 checks passed
@raph-luc raph-luc deleted the maint/more-details-deprecate-warn branch May 22, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate show_gui argument of launch_fluent
6 participants