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

Embetter pofile sorting #300

Merged
merged 4 commits into from Feb 2, 2016

Conversation

Projects
None yet
5 participants
@akx
Member

akx commented Dec 23, 2015

I think sorting message locations irrespective of the sort flags is alright.

The only UC I can think of where that wouldn't be desired might be trying to read a .po and emitting an identical copy of it, but I don't think Babel does that as it is anyway?

@codecov-io

This comment has been minimized.

codecov-io commented Dec 23, 2015

Current coverage is 89.45%

Merging #300 into master will increase coverage by +0.04% as of 766cec1

@@            master    #300   diff @@
======================================
  Files           23      23       
  Stmts         3740    3747     +7
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3344    3352     +8
  Partial          0       0       
+ Missed         396     395     -1

Review entire Coverage Diff as of 766cec1

Powered by Codecov. Updated on successful CI builds.

@akx akx force-pushed the akx:pofile-sort branch from 930fb65 to 0c10149 Dec 24, 2015

@akx akx force-pushed the akx:pofile-sort branch 2 times, most recently from 94bff46 to 0b70da8 Dec 24, 2015

@akx akx added this to the Babel 2.3/3.0 milestone Jan 3, 2016

akx added some commits Dec 23, 2015

@akx akx force-pushed the akx:pofile-sort branch from 0b70da8 to edce8ee Jan 4, 2016

@jtwang

This comment has been minimized.

jtwang commented on tests/messages/test_pofile.py in edce8ee Jan 26, 2016

Totally not your fault or your problem to fix, but ugh wrt giant methods resulting in non-intuitive 'unit' tests.

Please consider pulling the sort logic into a separate method. It's not a lot of logic, so it's a bit questionable as to whether or not it's worth it, but I think being able to have sane and full unit test coverage maaaaaaybe makes the answer 'yes'.

@jtwang

This comment has been minimized.

jtwang commented on bc59c9c Jan 27, 2016

Sure, why not. :)

@jtwang

This comment has been minimized.

jtwang commented on 4f60b3e Jan 27, 2016

OK, now I have a stronger argument for DRYing up the sorting logic into its own method. Plus, it might bump up code coverage. :)

@akx

This comment has been minimized.

Member

akx commented Jan 29, 2016

@jtwang DRYed! Good point.

for comment in message.user_comments:
_write_comment(comment)
_write_message(message, prefix='#~ ')
_write('\n')
def _sort_messages(messages, sort_by_message, sort_by_file):

This comment has been minimized.

@jtwang

jtwang Jan 29, 2016

Contributor

Test please and docs. No default values for sort flags?

@akx akx force-pushed the akx:pofile-sort branch from b3a59c1 to 65ce160 Feb 1, 2016

@akx

This comment has been minimized.

Member

akx commented Feb 1, 2016

@jtwang: I improved the signature of the helper function a little more -- no good reason to have two mutually exclusive arguments!

Also, I don't think it warrants a separate test; that function's coverage should already be 100% by way of the other tests.

@jtwang

This comment has been minimized.

Contributor

jtwang commented Feb 2, 2016

ack 4f60b3e, bc59c9c, edce8ee, 65ce160

@akx totally disagree about testing, but I'll defer to your familiarity with this project. :)

Single arg change looks good (bah enums only in 3.4). Good to merge!

@akx

This comment has been minimized.

Member

akx commented Feb 2, 2016

Cheers @jtwang! :)

akx added a commit that referenced this pull request Feb 2, 2016

Merge pull request #300 from akx/pofile-sort
Embetter pofile sorting

@akx akx merged commit edd83c4 into python-babel:master Feb 2, 2016

5 of 6 checks passed

review/gitmate/manual This commit needs review.
Details
codecov/patch 100.00% of diff hit (target 80.00%)
Details
codecov/project 89.45% (+0.05%) compared to c1ae655 at 89.40%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
review/gitmate/commit All is well! :)
Details

@akx akx deleted the akx:pofile-sort branch Feb 2, 2016

@pyup-bot pyup-bot referenced this pull request Jan 6, 2017

Merged

Update babel to 2.3.4 #13

@pyup-bot pyup-bot referenced this pull request Jan 31, 2017

Open

Update babel to 2.3.4 #28

@pyup-bot pyup-bot referenced this pull request Apr 11, 2017

Open

Initial Update #3

@pyup-bot pyup-bot referenced this pull request May 12, 2017

Closed

Initial Update #43

@pyup-bot pyup-bot referenced this pull request Jul 4, 2017

Merged

Initial Update #2

@pyup-bot pyup-bot referenced this pull request Nov 3, 2017

Closed

Update babel to 2.5.1 #424

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