Skip to content

Allow file locations without line numbers. #279

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

Merged
merged 1 commit into from
Dec 20, 2015

Conversation

fschulze
Copy link
Contributor

@fschulze fschulze commented Nov 4, 2015

I'm proposing that locations without line numbers should work. Currently they are just swallowed. Other tools like msgmerge from gettext keep them as they are. Locations without line numbers are useful to reduce the number of differences when templates and code are edited without changing any of the translations.
The current implementation might break backward compatibility for other code that uses pofile from Babel.
Another possible solution would be to just keep unparsed locations as regular comments, but that is quite a bit more complex to handle correctly in my opinion.

@codecov-io
Copy link

Current coverage is 83.54%

Merging #279 into master will increase coverage by +0.02% as of 6dc2e0c

@@            master    #279   diff @@
======================================
  Files           22      22       
  Stmts         3812    3817     +5
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3184    3189     +5
  Partial          0       0       
  Missed         628     628       

Review entire Coverage Diff as of 6dc2e0c

Powered by Codecov. Updated on successful CI builds.

@sils
Copy link
Member

sils commented Nov 4, 2015

@fschulze hey, thanks for submitting this PR! I hope I'll have time to take a look at this within a reasonable delay (I'm off for GSoC mentor summit from tomorrow), if not please ping me. This project is lacking review manpower, if you'd like to join we'd appreciate any help properly reviewing other PRs!

locs = []
for filename, lineno in message.locations:
if lineno:
locs.append(u'%s:%d' % (filename.replace(os.sep, '/'), lineno))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filename.replace(os.sep, '/') duplication could be moved to a filename = filename.replace(os.sep, '/') statement.

@akx
Copy link
Member

akx commented Dec 20, 2015

👍 for this; allowing omitting line numbers altogether makes diffs to .po files much nicer to read.

(Setting lineno to 0 works too, but filename:0 isn't as elegant as no lineno altogether.)

@sils
Copy link
Member

sils commented Dec 20, 2015

@akx thanks!

sils added a commit that referenced this pull request Dec 20, 2015
Allow file locations without line numbers.
@sils sils merged commit 6b6546b into python-babel:master Dec 20, 2015
@sils
Copy link
Member

sils commented Dec 20, 2015

added note to CHANGES file: https://github.com/python-babel/babel/wiki/CHANGES

@fschulze
Copy link
Contributor Author

@akx and @sils1297 : thanks for the review and merge!

@fschulze
Copy link
Contributor Author

Just curious, shouldn't the CHANGES file be updated and not only the wiki entry?

@akx
Copy link
Member

akx commented Dec 21, 2015

@fschulze: As I understand the CHANGES file in the repo will get updated at release, see #263

@sils
Copy link
Member

sils commented Dec 21, 2015

@fschulze updating CHANGES in each PR leads to lots of conflicts, in some cases maintainers check the commit log and compile the file from that but we have lots of merge commits cluttering the history here and no time to make every contributor rebase his PR so the wiki page is right now the most convenient option :/

@fschulze
Copy link
Contributor Author

Yeah, I know the pain of conflicts for CHANGES. Thanks for the clarification.

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

Successfully merging this pull request may close these issues.

4 participants