-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Update mui-language-picker and font scripts #3060
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3060 +/- ##
==========================================
+ Coverage 75.05% 84.15% +9.10%
==========================================
Files 269 47 -222
Lines 10387 4967 -5420
Branches 1227 587 -640
==========================================
- Hits 7796 4180 -3616
+ Misses 2231 643 -1588
+ Partials 360 144 -216
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Reviewed 8 of 8 files at r2.
Reviewable status: 6 of 8 files reviewed, 4 unresolved discussions (waiting on @imnasnainaec)
maintenance/Dockerfile
line 42 at r2 (raw file):
COPY --chown=user:user scripts/*.py ${SCRIPT_DIR} COPY --chown=user:user scripts/*.sh ${SCRIPT_DIR} COPY --chown=user:user scripts/*.txt ${SCRIPT_DIR}
Text files are not scripts so I would prefer to not copy all .txt
files to the scripts directory. I think the previous structure was better, only, it seems now that only the mui_language_picker_fonts.txt
file needs to be copied.
Code quote:
COPY --chown=user:user scripts/*.txt ${SCRIPT_DIR}
maintenance/scripts/get_fonts.py
line 53 at r2 (raw file):
nargs="*", metavar="SCRIPT", help="List of script tags for which fonts should be downloaded.",
This is confusing with the scripts in a script
directory! This is a due to the nature of the problem, not the design - I cannot think of a better name for the option.
If you also found it confusing we could rename the scripts
directory to bin
- more *nix like anyway and the Python scripts are in ~/.local/bin
in the maintenance container. Such a rename is probably best left to a different PR.
Code quote:
parser.add_argument(
"--scripts",
"-s",
nargs="*",
metavar="SCRIPT",
help="List of script tags for which fonts should be downloaded.",
maintenance/scripts/get_fonts.py
line 207 at r2 (raw file):
exit(1) offline: bool = args.langs or args.scripts
I found this variable name misleading - it is not True
if we are currently offline but if we are building the font data structures for use offline. It took me a while to recall that is the use model. I recommend changing the variable name or adding a comment where it is defined to explain its meaning. I can't think of a name offhand that conveys the meaning without being very long. Perhaps get_for_offline
? for_offline_use
? (not great)
Code quote:
offline: bool = args.langs or args.scripts
scripts/get_fonts_dev.py
line 5 at r2 (raw file):
""" Runs maintenance/scripts/get_fonts.py with dev arguments for -f and -o. Run this script with -U/--update whenever the MLP version is updated.
spell out MLP
Code quote:
MLP
scripts/get_fonts_dev.py
line 82 at r2 (raw file):
font_lines.sort() with open(mlp_font_list, "w") as fonts_file: fonts_file.writelines(font_lines)
This is writing to a tracked file, mlp_font_list
. This suggests that the mlp_font_list
is not really source code but a derived file. It would be better to add the mlp_font_list
to .gitignore
and run this script when the maintenance container is built. We should discuss this since I have not fully worked out in my mind the implications. If we keep the process as it is, then the --update
command should not allow the --scripts
or --langs
options. It is starting to sound like it should be a separate script.
Code quote:
with open(mlp_font_list, "w") as fonts_file:
fonts_file.writelines(font_lines)
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.
Reviewable status: 3 of 8 files reviewed, 2 unresolved discussions (waiting on @jmgrady)
maintenance/Dockerfile
line 42 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
Text files are not scripts so I would prefer to not copy all
.txt
files to the scripts directory. I think the previous structure was better, only, it seems now that only themui_language_picker_fonts.txt
file needs to be copied.
Done.
maintenance/scripts/get_fonts.py
line 207 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
I found this variable name misleading - it is not
True
if we are currently offline but if we are building the font data structures for use offline. It took me a while to recall that is the use model. I recommend changing the variable name or adding a comment where it is defined to explain its meaning. I can't think of a name offhand that conveys the meaning without being very long. Perhapsget_for_offline
?for_offline_use
? (not great)
Done.
scripts/get_fonts_dev.py
line 82 at r2 (raw file):
Previously, jmgrady (Jim Grady) wrote…
This is writing to a tracked file,
mlp_font_list
. This suggests that themlp_font_list
is not really source code but a derived file. It would be better to add themlp_font_list
to.gitignore
and run this script when the maintenance container is built. We should discuss this since I have not fully worked out in my mind the implications. If we keep the process as it is, then the--update
command should not allow the--scripts
or--langs
options. It is starting to sound like it should be a separate script.
The update runs before any other options are used.
Yes, it should probably become part of the deployment.
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.
Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)
MLP v2
offline
option (2.1.5)maintenance/scripts/get_fonts.py
andscripts/get_fonts_dev.py
-s/--scripts
(alongside-l/--langs
)get_fonts_dev
argument-U/--update
for updatingmui_language_picker_fonts.txt
This change is