Skip to content

jslexer.py: Change jsx_tag regex again #396

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

Merged
merged 1 commit into from
Oct 24, 2016
Merged

jslexer.py: Change jsx_tag regex again #396

merged 1 commit into from
Oct 24, 2016

Conversation

karloskar
Copy link
Contributor

We've had some syntactic variations that my previous PR, #392, did not
account for. One case had a component as a prop that tripped up the
regex.

@akx
Copy link
Member

akx commented Apr 28, 2016

@karloskar CI seems to be failing. Also, I think the test case is maybe a little too verbose? Would be better to test that extraction still works as it should, not that the lexer knows about every atom :)

@karloskar
Copy link
Contributor Author

I'll have to admit that I'm maybe in to deep here :)

I'm not that familiar with neither the codebase nor Travis but from what I can tell the error stems from Codecov complaining on some missing file. Not really sure what I can do about that :/

And I agree on the verbosity of the test. I had a hard time figuring out something that took in account all the different syntactic variants that I've seen in other peoples issues and that we are using. Would separating them to different methods be a suitable way forward? All tips are appreciated!

@akx akx self-assigned this Apr 28, 2016
@akx
Copy link
Member

akx commented May 17, 2016

Hey @karloskar -- sorry for the delay in getting back to you!

Could you list some of the cases you'd need to handle in your codebase?

@karloskar
Copy link
Contributor Author

Sure! Thanks for taking a look!

The ones in the test file covers all of our variations right now. And despite a rather high tempo and a growing code base the last couple of weeks we haven't found a need for anything else.

As children to a component

<option value="{ true }">{ i18n._('String') }</option>

As an "interpolated" prop

<component value={ i18n._('String') } />

And this variant where there are components as props that may or may not contain translatable strings themselves.

<component prop={ <otherComponent /> } data={ {active: true} }>
  <btn text={ i18n._('String') } />
</component>

The first case added to the test file (multiple option tags) was taken from this issue python-babel/flask-babel#79.

@codecov-io
Copy link

codecov-io commented Oct 21, 2016

Current coverage is 90.11% (diff: 100%)

Merging #396 into master will decrease coverage by 0.05%

@@             master       #396   diff @@
==========================================
  Files            24         24          
  Lines          3954       3954          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           3565       3563     -2   
- Misses          389        391     +2   
  Partials          0          0          

Powered by Codecov. Last update ead6ed9...be0d8cb

@akx
Copy link
Member

akx commented Oct 21, 2016

Sorry it took me so long to get back to this!

Can you rebase this on top of the current master? We've disabled OSX CI in the meanwhile since it seems to be broken, but this branch doesn't currently have that CI config :)

@karloskar
Copy link
Contributor Author

Can't say I really knew what I did. First time rebasing for a pull request update. Would the conflicts be resolvable by you or do you want me to change anything?

@akx
Copy link
Member

akx commented Oct 24, 2016

Ahh, yeah, something went wrong there alright. Mind if I just grab your patches and create a new PR out of them, @karloskar? :) Will naturally keep your Author credit.

@karloskar
Copy link
Contributor Author

I do not mind at all. Please do as you see fit!

@akx
Copy link
Member

akx commented Oct 24, 2016

Oh, wait, @karloskar: I think you might be able to resolve this on your end, as the pt2 commit d2fc566 applies cleanly :)

# (if you don't yet have this repo as the upstream)
git remote add upstream https://github.com/python-babel/babel.git
# ensure your upstream remote is up to date
git fetch upstream
# checkout this branch
git checkout feature/evolve-jsx-extraction-pt2
# keep a stash branch around for possible disaster recovery purposes
git branch orig-jsx-extraction
# reset it to upstream/master
git reset --hard upstream/master
# apply the pt2 patch
git cherry-pick d2fc566f66272c026c50472e20d4e23748c59279
# force-push the branch (now with only one commit on top of upstream/master)
git push -f origin HEAD

Feel free to remove the disaster recovery branch at your leisure.

We've had some syntactic variations that my previous PR, #392, did not
account for. One case had a component as a prop that tripped up the
regex.
@karloskar
Copy link
Contributor Author

Like that @akx?

@akx
Copy link
Member

akx commented Oct 24, 2016

@karloskar Yes, brilliant :D

@akx akx merged commit 82cb625 into python-babel:master Oct 24, 2016
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.

4 participants