Don't determine python-format flag implicitly by JonathanRRogers · Pull Request #80 · python-babel/babel · GitHub
Skip to content

Don't determine python-format flag implicitly#80

Open
JonathanRRogers wants to merge 1 commit into
python-babel:masterfrom
JonathanRRogers:master
Open

Don't determine python-format flag implicitly#80
JonathanRRogers wants to merge 1 commit into
python-babel:masterfrom
JonathanRRogers:master

Conversation

@JonathanRRogers

Copy link
Copy Markdown

This extends the extractor interface as discussed in #35

@sils

sils commented Aug 5, 2015

Copy link
Copy Markdown
Member

@sils

sils commented Sep 24, 2015

Copy link
Copy Markdown
Member

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

@codecov-io

codecov-io commented Oct 20, 2015

Copy link
Copy Markdown

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

sils commented Oct 20, 2015

Copy link
Copy Markdown
Member

@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

sils commented Oct 20, 2015

Copy link
Copy Markdown
Member

@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

sils commented Oct 20, 2015

Copy link
Copy Markdown
Member

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
Copy Markdown
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

sils commented Oct 21, 2015

Copy link
Copy Markdown
Member

@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

sils commented Oct 21, 2015

Copy link
Copy Markdown
Member

Test coverage looks good now, thanks.

@sils

sils commented Oct 21, 2015

Copy link
Copy Markdown
Member

@JonathanRRogers

Copy link
Copy Markdown
Author

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

@akx

akx commented Feb 12, 2016

Copy link
Copy Markdown
Member

@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
Copy Markdown
Author

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

@miroslavrehor

Copy link
Copy Markdown

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