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 lexer improvements #773

Closed

Conversation

gitaarik
Copy link
Contributor

Add option parse_template_string that improves parsing of javascript templates expressions, like:

`${gettext('my text')}`

And it also works with nested expressions like:

${`some text ${gettext('my text')} more text`}

You have to enable template string extraction with an option in the option_map:

extract_from_dir(
    # ...
    options_map={
        '**': {'parse_template_string': True}
    }
)

Then it should work.

On this branch I also fixed another issue that I was facing. In a JS library that I use, there was regex with a / inside a character class [a-z]. The babel JS lexer thought this closed the regex, but JavaScript allows this as a normal match character within a character class, and does not close the regex. I updated the regex so that it will cover this exception.

@codecov
Copy link

codecov bot commented May 22, 2021

Codecov Report

Merging #773 (2637c60) into master (7ebdf5a) will not change coverage.
The diff coverage is n/a.

❗ Current head 2637c60 differs from pull request most recent head b03eae0. Consider uploading reports for the commit b03eae0 to get more accurate results
Impacted file tree graph

@@      Coverage Diff      @@
##   master   #773   +/-   ##
=============================
=============================

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 7ebdf5a...b03eae0. Read the comment docs.

@gitaarik gitaarik force-pushed the javascript-improvements branch 2 times, most recently from 723e87d to 1034a47 Compare June 9, 2021 07:31
@gitaarik
Copy link
Contributor Author

gitaarik commented Jun 9, 2021

Hi @akx . I've been working on this feature for a while. In my setup I need these changes. However I have some problems when using my own fork in my project. So I have some questions regarding using a fork of babel in my project, and also what needs to be done to get this merged, so I don't have to use my fork anymore.

In my project, which uses Pipenv. Inside the Pipfile, I use a VCS descriptor to specify my python-babel fork:

babel = {editable = true, ref = "javascript-improvements-1", git = "https://github.com/gitaarik/babel"}

This works partly. When I run python-babel's babel.messages.extract.extract_from_dir, it finds gettext() functions that it didn't found before.

However, when I run my Django devserver, on some requests (not all of them, I'm not sure yet why it only happens on some requests), the request fails with a exception coming from python-babel:

RuntimeError: The babel data files are not available. This usually happens because you are using a source checkout from Babel and you did not build the data files.  Just make sure to run "python setup.py import_cldr" before installing the library.

I noticed that when I go into the virtualenv directory (which is created by Pipenv and can be retrieved with pipenv --venv) and then from there go to src/babel/, I can run the command python setup.py import_cld to generate the locale files and then it seems to work.

I will have to look into the possibilities to integrate this in my project in a way that the locale files would be generated when I build and deploy my project. In the mean time it would be good for us if the changes would eventually be merged into the official package.

So I have two questions:

  • Are there easier ways of using my fork in my project, for development and deployment?
  • What would be needed to get these changes merged into the official package, so that we don't have to make things complicated with a fork?

@gitaarik
Copy link
Contributor Author

gitaarik commented Jun 9, 2021

Closing this MR now, because I splitted this one up in 2 separate MRs:

Please review and reply in the appropriate new MRs.

@gitaarik gitaarik closed this Jun 9, 2021
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.

1 participant