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

Make catalog header updating an option #320

Merged
merged 1 commit into from Jan 8, 2016
Merged

Conversation

akx
Copy link
Member

@akx akx commented Jan 8, 2016

The change in e0e7ef1 had an unexpected and likely undesired effect
when updating catalogs with, e.g. translator names in the header comment.

It's better to make the updating an option and revert back to the pre-2.2
behavior by default.

Fixes #318

@codecov-io
Copy link

Current coverage is 88.29%

Merging #320 into master will increase coverage by +0.01% as of cd96127

@@            master    #320   diff @@
======================================
  Files           22      22       
  Stmts         3595    3597     +2
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3174    3176     +2
  Partial          0       0       
  Missed         421     421       

Review entire Coverage Diff as of cd96127

Powered by Codecov. Updated on successful CI builds.

@sils
Copy link
Member

sils commented Jan 8, 2016

@akx needs more test coverage.

@akx
Copy link
Member Author

akx commented Jan 8, 2016

@sils1297 The lack of test coverage would be fixed by merging #311. Right now the distutils versions of command code are not tested at all, but the other copies are.

I published the option only in the distutils code anticipating that merge.

@ajaeger
Copy link

ajaeger commented Jan 8, 2016

Tested this locally and it works as expected - fixes issue #318 in my setup. Thanks!

@akx
Copy link
Member Author

akx commented Jan 8, 2016

I can also just remove the new option in the distutils code, but that seems like doing it just to make Codecov happy, and we're not writing code for CI robots. :)

@sils
Copy link
Member

sils commented Jan 8, 2016

@akx then lets merge #311 first?

@akx akx force-pushed the update-option branch 2 times, most recently from 59831b5 to 672f96c Compare January 8, 2016 09:27
@akx
Copy link
Member Author

akx commented Jan 8, 2016

Hey @ajaeger, if you have the time, could you check that this branch still works for your UC (after it was rebased)? Thank you!

The change in e0e7ef1 had an unexpected and likely undesired effect
when updating catalogs with, e.g. translator names in the header comment.

It's better to make the updating an option and revert back to the pre-2.2
behavior by default.

Fixes python-babel#318
akx added a commit that referenced this pull request Jan 8, 2016
Make catalog header updating an option
@akx akx merged commit 2e20745 into python-babel:master Jan 8, 2016
@akx akx deleted the update-option branch January 8, 2016 13:07
@ajaeger
Copy link

ajaeger commented Jan 8, 2016

@akx tested current head that includes the patch and it works fine. Thanks

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.

None yet

5 participants