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

javascript extract improvements #939

Merged

Conversation

johanneswilm
Copy link
Contributor

  • Improved javascript template string expression extracting
  • handle line numbers + add tests for JS template strings

@johanneswilm
Copy link
Contributor Author

Hey @akx I have address the issues you mentioned in #792 . There are now tests, the style is more in line with your code and line numbers are handled correctly. Would it be possible to merge this or do you need me to do something more?
PS: I will try to contact you in some other way as well, given that you wrote somewhere here that you muted github notifications.

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #939 (8a6c452) into master (82c41cc) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #939      +/-   ##
==========================================
+ Coverage   91.56%   91.62%   +0.05%     
==========================================
  Files          23       23              
  Lines        4244     4273      +29     
==========================================
+ Hits         3886     3915      +29     
  Misses        358      358              
Impacted Files Coverage Δ
babel/messages/extract.py 95.23% <100.00%> (+0.50%) ⬆️
babel/messages/jslexer.py 86.86% <100.00%> (-0.14%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@johanneswilm
Copy link
Contributor Author

Hey @akx ,
is there anything you need me to do or does this look ok for merging?

Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Some minor changes requested...

babel/messages/jslexer.py Show resolved Hide resolved
babel/messages/extract.py Outdated Show resolved Hide resolved
babel/messages/extract.py Outdated Show resolved Hide resolved
babel/messages/extract.py Outdated Show resolved Hide resolved
@johanneswilm
Copy link
Contributor Author

@akx OK, all changes made as you requested.

As for:

Also, maybe it should be in options all in all, so as not to change the extractor function signature?

This is up to you to decide. I can see arguments doing either (it is an option, but it's different from the other ones). Feel free to make that change if you think it's better to do it that way. I have left it as it was for now.

Please let me know if you need me to do anything else so that we can get this merged ASAP!

…wilm/babel into javascript-extract-improvements
@johanneswilm johanneswilm requested a review from akx January 6, 2023 19:23
Copy link
Member

@akx akx 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 for the contribution!

@akx akx merged commit d425f86 into python-babel:master Jan 6, 2023
@johanneswilm johanneswilm deleted the javascript-extract-improvements branch January 6, 2023 20:19
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.

3 participants