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

Generate tutorial addons #3638

Merged
merged 20 commits into from
Jun 10, 2022
Merged

Generate tutorial addons #3638

merged 20 commits into from
Jun 10, 2022

Conversation

bakulf
Copy link
Collaborator

@bakulf bakulf commented May 28, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2022

Codecov Report

Merging #3638 (a4d1e1b) into main (ea235cb) will decrease coverage by 0.85%.
The diff coverage is 27.98%.

@@            Coverage Diff             @@
##             main    #3638      +/-   ##
==========================================
- Coverage   61.86%   61.00%   -0.86%     
==========================================
  Files         270      277       +7     
  Lines       17812    18104     +292     
  Branches     8907     8981      +74     
==========================================
+ Hits        11019    11044      +25     
- Misses       5107     5337     +230     
- Partials     1686     1723      +37     
Flag Coverage Δ
functional_tests 26.13% <4.63%> (-0.93%) ⬇️
lottie_tests 56.33% <ø> (ø)
qml_tests 10.07% <ø> (-0.12%) ⬇️
unit_tests 70.83% <25.94%> (-2.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/addons/addon.h 0.00% <0.00%> (ø)
src/addons/addoni18n.cpp 0.00% <0.00%> (ø)
src/constants.h 50.00% <0.00%> (-7.15%) ⬇️
src/featureslist.h 0.00% <ø> (ø)
src/inspector/inspectorhandler.cpp 21.66% <0.00%> (+0.18%) ⬆️
src/models/guide.h 0.00% <ø> (-100.00%) ⬇️
src/models/guidemodel.h 0.00% <ø> (-100.00%) ⬇️
src/models/tutorial.h 50.00% <ø> (ø)
src/models/tutorialmodel.h 100.00% <ø> (ø)
src/addons/addon.cpp 1.38% <1.38%> (ø)
... and 52 more

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 ea235cb...a4d1e1b. Read the comment docs.

@bakulf bakulf force-pushed the addonTutorial branch 29 times, most recently from 610d823 to 591856d Compare May 31, 2022 15:12
@bakulf bakulf requested a review from flodolo June 5, 2022 15:48
@flodolo
Copy link
Collaborator

flodolo commented Jun 5, 2022

I think generate_ts.sh should generate the .ts files also for the addons, and the translations workflow should upload all of them?

We also need to define the behavior between main and branches. We need to support branches, in case an add-on gets removed.

Do we merge the ts if there is already a file with the same name? That seems like the most sensible approach.

@flodolo
Copy link
Collaborator

flodolo commented Jun 5, 2022

Not 100% sure it works or how the resulting XLIFF looks like, but this is the idea
https://github.com/flodolo/mozilla-vpn-client/commit/2a0f7f9399ee311ec7fdeb5875ccaeeb91c9368c

Also, other Python clean-ups: https://github.com/flodolo/mozilla-vpn-client/commit/fc135e4df450bce75e5c1b2af6745b9f5246a4eb

BTW, how are you going to import back translations? There's nothing in the current PR.

Copy link
Collaborator

@oskirby oskirby left a comment

Choose a reason for hiding this comment

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

Overall, I think this is pretty good and aside from a few minor changes I don't see any major problems with merging this work. We should still have enough time for these changes to settle before the fix and code freeze deadlines.

scripts/ci/jsonSchemas/addon.json Outdated Show resolved Hide resolved
scripts/utils/generate_strings.py Show resolved Hide resolved
src/addonmanager.cpp Outdated Show resolved Hide resolved
src/addonmanager.cpp Outdated Show resolved Hide resolved
src/cmake/linux.cmake Outdated Show resolved Hide resolved
scripts/addon/build.py Outdated Show resolved Hide resolved
scripts/addon/build.py Outdated Show resolved Hide resolved
@flodolo
Copy link
Collaborator

flodolo commented Jun 10, 2022

Note: as mentioned before, this is still missing the "import strings" part. But I'd be OK with keeping that as a separate follow-up, when you actually have the strings in the repo to use.

@bakulf bakulf requested a review from oskirby June 10, 2022 16:13
Copy link
Collaborator

@oskirby oskirby left a comment

Choose a reason for hiding this comment

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

Looks good!

@bakulf bakulf merged commit 2679172 into main Jun 10, 2022
@bakulf bakulf deleted the addonTutorial branch June 10, 2022 19:14
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.

4 participants