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

Improved javascript template string expression extracting #792

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gitaarik
Copy link
Contributor

@gitaarik gitaarik commented Jun 9, 2021

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.

@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 meantime 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?

@akx
Copy link
Member

akx commented Jan 28, 2022

Hi @gitaarik, sorry for the latency. I had somehow inadvertently turned all notifications about Babel off (or maybe had just dropped the ball on this one).

Could you rebase this on master and maybe add a test that shows how this works?

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.

Beyond the other comments, I'd appreciate some tests :)


fake_file_obj = io.BytesIO(expression_contents.encode())

for item in extract_javascript(fake_file_obj, keywords, comment_tags, options):
Copy link
Member

Choose a reason for hiding this comment

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

How does this interact with e.g. line numbering?

Copy link
Contributor

Choose a reason for hiding this comment

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

line numbering has been fixed in #939. There is now also a test for that.

Comment on lines +655 to +692
def parse_template_string(template_string, fileobj, keywords, comment_tags, options):

prev_character = None
level = 0
inside_str = False
expression_contents = ''

for character in template_string[1:-1]:

if not inside_str and character in ('"', "'", '`'):
inside_str = character
elif inside_str == character and prev_character != r'\\':
inside_str = False

if level:
expression_contents += character

if not inside_str:

if character == '{' and prev_character == '$':
level += 1

elif level and character == '}':

level -= 1

if level == 0 and expression_contents:

expression_contents = expression_contents[0:-1]

fake_file_obj = io.BytesIO(expression_contents.encode())

for item in extract_javascript(fake_file_obj, keywords, comment_tags, options):
yield item

expression_contents = ''

prev_character = character
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the empty lines within the blocks, so the spacing of this code (which is quite airy 😄 ) matches the rest of the codebase?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in #939.

@johanneswilm
Copy link
Contributor

Beyond the other comments, I'd appreciate some tests :)

Tests have been added in #939.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants