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

Frontend: Add multi-domain support to compile_catalog command #335

Merged
merged 1 commit into from Jan 29, 2016

Conversation

@ansiwen
Copy link
Contributor

@ansiwen ansiwen commented Jan 27, 2016

Some projects have their translations split up into several text
domains within one package. This change adds the possibility to
specify a space seperated list of domains in the configuration, like
'setup.py compile_catalog --domain="foo bar"', for instance.

@gitmate-bot gitmate-bot added the size/L label Jan 27, 2016
@codecov-io
Copy link

@codecov-io codecov-io commented Jan 27, 2016

Current coverage is 89.41%

Merging #335 into master will increase coverage by +0.01% as of 9db2cf3

@@            master    #335   diff @@
======================================
  Files           23      23       
  Stmts         3736    3740     +4
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3340    3344     +4
  Partial          0       0       
  Missed         396     396       

Review entire Coverage Diff as of 9db2cf3

Powered by Codecov. Updated on successful CI builds.

@ansiwen
Copy link
Contributor Author

@ansiwen ansiwen commented Jan 27, 2016

56% patch coverage? That's not fair, just because I had to indent the whole block? 😒

mo_files.append(self.output_file)
domains = self.domain.split()

for domain in domains:

This comment has been minimized.

@akx

akx Jan 27, 2016
Member

It would make for a cleaner patch and cleaner code if you just split def run_domain(self, domain) into a separate fn, I think?

This comment has been minimized.

@ansiwen

ansiwen Jan 27, 2016
Author Contributor

right, good point, will do that

for domain in domains:
self.__run_domain(domain)

def __run_domain(self, domain):

This comment has been minimized.

@akx

akx Jan 28, 2016
Member

Ah, please avoid double-underscore-prefixed names. Just _run_domain or run_domain would be fine!

This comment has been minimized.

@ansiwen

ansiwen Jan 28, 2016
Author Contributor

What's the reason for that? I learned, double-underscore-prefix automatically adds the classname as prefix, so you have a kind of "private" member.

This comment has been minimized.

@akx

akx Jan 28, 2016
Member

Sure, but that's also very nonintuitive behavior.

Even PEP 8 advises against using the mangly names unless you have a good reason.

As for the privateness viewpoint, Python's attitude generally is "we're all consenting adults here", i.e. no need to go to lengths to hide the member. The single underscore is already a signal that this is some sort of private API, and you should know what you're doing if you're calling or overriding this.

@akx
Copy link
Member

@akx akx commented Jan 28, 2016

@ansiwen Heh, don't worry about codecov's patch measurements. It's a cranky ol' thing sometimes.

Some projects have their translations split up into several text
domains within one package.  This change adds the possibility to
specify a space seperated list of domains in the configuration, like
'setup.py compile_catalog --domain="foo bar"', for instance.
@akx
Copy link
Member

@akx akx commented Jan 29, 2016

Thank you @ansiwen 😻

akx added a commit that referenced this pull request Jan 29, 2016
Frontend: Add multi-domain support to compile_catalog command
@akx akx merged commit 9edb9a0 into python-babel:master Jan 29, 2016
5 of 6 checks passed
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.41% (+0.01%) 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
@ansiwen ansiwen deleted the ansiwen:multi_domain branch Jan 29, 2016
@ansiwen
Copy link
Contributor Author

@ansiwen ansiwen commented Jan 29, 2016

@akx: Cool, thanks for the merge. When can I expect a release with this feature? (Since we need it for openstack.)

@sils
Copy link
Member

@sils sils commented Jan 29, 2016

@akx as there is demand for a release I'm in favour of doing that soon.

@ansiwen if @akx disagrees we can still do a development release that you can get via pypi if you specify the version exactly in your requirements.txt or install with --pre

@akx
Copy link
Member

@akx akx commented Jan 29, 2016

@ansiwen @sils1297 #330 will need to be implemented before release (it's a seriously short fix).

@ansiwen
Copy link
Contributor Author

@ansiwen ansiwen commented Feb 9, 2016

@sils1297 we will wait for a regular release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.