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

Don't determine python-format flag implicitly #80

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

Conversation

JonathanRRogers
Copy link

This extends the extractor interface as discussed in #35

@sils
Copy link
Member

sils commented Aug 5, 2015

Commit message: please add reason. See http://coala.readthedocs.org/en/latest/Getting_Involved/Writing_Good_Commits/#how-to-write-good-commit-messages for more info.

Please rebase, should fix the tests.

@sils
Copy link
Member

sils commented Sep 24, 2015

@JonathanRRogers still around? Please let us know if you need any assistance!

@codecov-io
Copy link

codecov-io commented Oct 20, 2015

Codecov Report

Merging #80 into master will increase coverage by 0.06%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   90.31%   90.37%   +0.06%     
==========================================
  Files          24       24              
  Lines        4129     4134       +5     
==========================================
+ Hits         3729     3736       +7     
+ Misses        400      398       -2
Impacted Files Coverage Δ
babel/messages/pofile.py 96.19% <ø> (ø) ⬆️
babel/messages/extract.py 96.42% <100%> (+1.15%) ⬆️
babel/messages/frontend.py 86.34% <100%> (ø) ⬆️
babel/messages/catalog.py 94.42% <75%> (-0.27%) ⬇️
babel/_compat.py 98.03% <0%> (-0.08%) ⬇️
babel/localedata.py 95.45% <0%> (+0.04%) ⬆️

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 7ed6cc5...70a4b3a. Read the comment docs.

@sils
Copy link
Member

sils commented Oct 20, 2015

@JonathanRRogers would you mind removing the merge commits and doing a rebase (git rebase --interactive will do the whole trick) instead? Also the test coverage is not sufficient for this to be merged. We only want to see your commits in the PR so we don't mix up anything making review complicated and error prone.

@sils
Copy link
Member

sils commented Oct 20, 2015

@JonathanRRogers from llooking shortly at the commits now, e.g. the first one needs an explanation in the commit message so it is clear what this is, also note that no commit should break the build because each of them needs to be revertible later individually. So you probably want to squash your "Fix tests" bug with the one where it belongs to.

@sils
Copy link
Member

sils commented Oct 20, 2015

oh and please ping me when you add new commits, I don't check all PRs regularly, a comment will summon me :)

@JonathanRRogers
Copy link
Author

Though I have been using git for years, I have never needed to rebase and I haven't used GitHub much. I looked around GitHub for documentation relating to rebase to no avail. I ran "git rebase --interactive" and it had no effect. Perhaps you can point me to an example of what you want exactly.

I have added a test to cover the extract function interface change I made and all the checks have passed.

@sils
Copy link
Member

sils commented Oct 21, 2015

@JonathanRRogers GitHub isn't really rebase friendly. If you need it depends on what workflow you are using. Using rebase has the advantage of making the commit history more maintenance friendly, you don't have all those merge commits cluttering it. This eases review and thus helps reducing bugs because a merge commit is close to non-reviewable.

You'll want to pull the current master first from this repository, then checkout your branch and do a git rebase --interactive master, git will show you a document in your editor where you can specify for each commit what you want to do with it (edit, squash, reword commit message) and if you remove a line you remove that commit.

If you need anything else, please contact me simply on https://gitter.im/python-babel/babel , I'm always happy to help :)

@sils
Copy link
Member

sils commented Oct 21, 2015

Test coverage looks good now, thanks.

@sils
Copy link
Member

sils commented Oct 21, 2015

@JonathanRRogers
Copy link
Author

If anyone's actually interested in this enhancement, let me know.

@akx
Copy link
Member

akx commented Feb 12, 2016

@JonathanRRogers Hey, sorry for the delay with the patch 😿

This looks mostly like a nice patch, though adding an additional return value to extractors should probably be dealt with due care. In particular, all Babel-internal code paths should be checked to accept both 5-tuples and 6-tuples from extractors. (Looks like you've done that already, though?)

Even so, I'm afraid that external code that bypasses extract_from_dir might expect only 5-tuples from Babel-internal (or other...) extractors, which might mean that adding a sixth retval to extractors is a semver major change. That's not to say making a semver major change is bad; it's just another big step, and if we do something like that, it would be worth it to do other semver major changes in the same go.

For instance, formalizing the output interface for extractors from just regular tuples to namedtuples might be a worthy step to do at that point.

All that said, if you could rebase this patch on top of the current master, we could continue the discussion? :)

@JonathanRRogers
Copy link
Author

I'm sorry I've neglected this pull request for so long. I have rebased it again.

@miroslavrehor
Copy link

I would appriciate this functionality very much. For my own extractor (angularjs) I need the possibility to change the flag ("angularjs-format").

chuckyblack added a commit to chuckyblack/pybabel-angularjs that referenced this pull request Sep 27, 2018
During extraction, Message instances can be created with the
"python-format" flag, indicating that the message string contains Python
percent-formatting placeholders. To avoid setting the flag erroneously
because the string source is not Python code or otherwise is not
expected to contain such placeholders, the extractor interface must be
extended to allow extractor functions to indicate which flags are
valid.

Fixes python-babel#35
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.

6 participants