Don't determine python-format flag implicitly

[JonathanRRogers]

trafficstars

This extends the extractor interface as discussed in https://github.com/mitsuhiko/babel/issues/35

[sils]

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]

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

[codecov-io]

Codecov Report

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

@@            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% <ø> (ø)	:arrow_up:
babel/messages/extract.py	96.42% <100%> (+1.15%)	:arrow_up:
babel/messages/frontend.py	86.34% <100%> (ø)	:arrow_up:
babel/messages/catalog.py	94.42% <75%> (-0.27%)	:arrow_down:
babel/_compat.py	98.03% <0%> (-0.08%)	:arrow_down:
babel/localedata.py	95.45% <0%> (+0.04%)	:arrow_up:

---

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]

@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]

@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]

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

[JonathanRRogers]

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]

@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]

Test coverage looks good now, thanks.

[sils]

and here's a tutorial: https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase-i

[JonathanRRogers]

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

[akx]

@JonathanRRogers Hey, sorry for the delay with the patch :crying_cat_face:

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]

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

[miroslavrehor]

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