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

Rename translation function alias #428

Merged
merged 8 commits into from
Mar 1, 2021

Conversation

sngyo
Copy link
Contributor

@sngyo sngyo commented Feb 23, 2021

Corresponds to #424

In this project, _ is used as an alias for trans.gettext although _ is commonly used as a placeholder for ignored variables or loop variables in Python.
In this PR, I'm trying to rename _ to _i18n as discussed in #424, it would be clearer and more useful to avoid conflicts between these two uses.

P.S.
This is the first time for me to make a contribution, so I would appreciate it if you could kindly point out any inadequacies/advice 😉

TODO

  • update CONTRIBUTING.rst since the link is outdated.
  • there are bunch of strings which are not given to _i18n in serverapp.py

@sngyo sngyo marked this pull request as draft February 23, 2021 03:35
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sngyo - thanks for doing this. My primary comment is regarding constraining this to only the existing _() functions.

@@ -1714,17 +1714,17 @@ def shutdown_no_activity(self):

seconds_since_active = \
(utcnow() - self.web_app.last_activity()).total_seconds()
self.log.debug(_i18n("No activity for %d seconds.",
seconds_since_active))
self.log.debug("No activity for %d seconds.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be introducing new localized strings in this exercise. Only those statements wrapped in _() should be replaced with _i18n()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this comment is on the wrong line now and it looks like things were reverted, but I think the point is still the same - let's not introduce new strings for localization bundles in this PR. I think there are a few other cases in serverapp.py where this occurred.

Copy link
Contributor Author

@sngyo sngyo Feb 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kevin-bates,
thanks for the advice, I'll revert the last two commits later! 😄
--> DONE!

@@ -10,4 +10,4 @@
# Set up message catalog access
base_dir = os.path.realpath(os.path.join(__file__, '..', '..'))
trans = gettext.translation('notebook', localedir=os.path.join(base_dir, 'notebook/i18n'), fallback=True)
_ = trans.gettext
_i18n = trans.gettext
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goanpeca, @Zsailer - do you know if server extensions use this _ alias to wrap localizable strings? If so, we may not want to fully replace it, but define two aliases for now. Since we may not know, it might be best to keep both.

@sngyo - I suspect we'll want to add some kind of commented directive such that the "unused global variable" warning is not emitted. (This is probably another reason why _ was used.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevin-bates

1. two alias

How do you think introducing a deprecation warning for _() as below?

import warnings

def _trans_gettext_helper(*args, **kwargs):
    warn_msg = "The alias `_()` will be deprecated. Use `_i18n()` instead."
    warnings.warn(warn_msg, UserWarning)
    return trans.gettext(*args, **kwargs)


_ = _trans_gettext_helper
_i18n = trans.gettext

2. skip unused global variable

I'll look for the method to ignore (skip) the warning for Github CodeScanning. There should be something like this (maybe it will work 😆 ):

_i18n = trans.gettext  # noqa: E640

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. two alias

I think it makes sense to add a deprecation warning for this. I see you're using UserWarning - I'm guessing due to the limitations for DeprecationWarning. I would suggest using FutureWarning for this instead since it more closely identifies with the intention. (We also used this warning level recently in jupyter_client.)

  1. skip unused global variable

I had linting on my mind when I made this suggestion. I'm not immediately finding linting-style directives for codeQL, so this probably isn't a big deal since codeQL is still succeeding. Sorry for the misdirection.

@codecov-io
Copy link

codecov-io commented Feb 25, 2021

Codecov Report

Merging #428 (5210fa2) into master (9b9b311) will increase coverage by 0.02%.
The diff coverage is 44.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
+ Coverage   77.46%   77.48%   +0.02%     
==========================================
  Files         105      105              
  Lines        8936     8942       +6     
  Branches      956      956              
==========================================
+ Hits         6922     6929       +7     
+ Misses       1677     1676       -1     
  Partials      337      337              
Impacted Files Coverage Δ
jupyter_server/serverapp.py 65.85% <33.33%> (ø)
jupyter_server/transutils.py 72.72% <57.14%> (-27.28%) ⬇️
jupyter_server/extension/application.py 72.44% <100.00%> (ø)
jupyter_server/services/contents/filemanager.py 66.95% <100.00%> (ø)
jupyter_server/services/contents/manager.py 88.32% <100.00%> (ø)
jupyter_server/services/kernels/handlers.py 58.24% <0.00%> (+1.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b9b311...5210fa2. Read the comment docs.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good - thank you @sngyo. (I'm assuming this is no longer WIP, but please let us know if that's not the case.)

@sngyo sngyo changed the title WIP: Rename translation function alias Rename translation function alias Feb 27, 2021
@sngyo sngyo marked this pull request as ready for review February 27, 2021 01:32
@sngyo
Copy link
Contributor Author

sngyo commented Feb 27, 2021

@kevin-bates Thank you for the review and advice! It's ready to be merged 😄

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks, @sngyo.

@kevin-bates kevin-bates merged commit d07853e into jupyter-server:master Mar 1, 2021
@kevin-bates
Copy link
Member

Thanks @sngyo - this is helpful!

@blink1073 blink1073 added this to the 1.5 milestone Mar 18, 2021
Zsailer added a commit to Zsailer/jupyter_server that referenced this pull request Nov 18, 2022
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.

None yet

5 participants