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

Reimplement POEntry.__eq__ method, based on __cmp__ #107

Closed
wants to merge 1 commit into from
Closed

Reimplement POEntry.__eq__ method, based on __cmp__ #107

wants to merge 1 commit into from

Conversation

ldaverio
Copy link

@ldaverio ldaverio commented Apr 5, 2021

Hello,

I've just come across a problem while trying to extract messages from a Pyramid web app using Mako (1.1.5) templates. As per the Pyramid tutorial, I use Lingua (4.1.4) to extract messages. The error message was:

  File "/my-project/venv/bin/pot-create", line 8, in <module>
    sys.exit(main())
  File "/my-project/venv/lib/python3.8/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/my-project/venv/lib/python3.8/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/my-project/venv/lib/python3.8/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/my-project/venv/lib/python3.8/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/my-project/venv/lib/python3.8/site-packages/lingua/extract.py", line 453, in main
    save_catalog(catalog, output)
  File "/my-project/venv/lib/python3.8/site-packages/lingua/extract.py", line 244, in save_catalog
    if old_catalog is not None and identical(catalog, old_catalog):
  File "/my-project/venv/lib/python3.8/site-packages/lingua/extract.py", line 234, in identical
    return a == b
  File "/my-project/venv/lib/python3.8/site-packages/lingua/extract.py", line 67, in __eq__
    r = super(POEntry, self).__eq__(other)
  File "/my-project/venv/lib/python3.8/site-packages/polib.py", line 1120, in __eq__
    return self.__cmp__(other) == 0
  File "/my-project/venv/lib/python3.8/site-packages/polib.py", line 1091, in __cmp__
    if msgstr_plural > othermsgstr_plural:
TypeError: '>' not supported between instances of 'dict' and 'dict'

Basically, pot-create tries to determine if the catalog needs to be updated, by comparing all entries between the existing catalog and the new one. Now, in polib, testing if two entries are equal is ultimately done via the __cmp__ method, which uses < and > operators for comparison.

The problem arises as the msgstr_plural fields returned by the Lingua extractor can be dicts, and so the <and > operators no longer apply. Comparing dict equality needs to be done diferently. In this version, I've reimplemented the __eq__ method so that it only uses !=operators.

Polib tests all succeed, to the change doesn't seem to break anything. The only thing that bothers me is code redundancy...

Laurent.

@codecov-io
Copy link

codecov-io commented Apr 5, 2021

Codecov Report

Merging #107 (8a7026a) into master (088ccd1) will decrease coverage by 0.92%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
- Coverage   95.22%   94.30%   -0.93%     
==========================================
  Files           1        1              
  Lines         838      860      +22     
==========================================
+ Hits          798      811      +13     
- Misses         40       49       +9     
Impacted Files Coverage Δ
polib.py 94.30% <91.66%> (-0.93%) ⬇️

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 088ccd1...8a7026a. Read the comment docs.

@izimobil
Copy link
Owner

Hi, thanks for your PR but this was a real bug in polib, I'm surprised it wasn't reported earlier !.

msgstr_plural is always a dict in polib, here's the correct fix: 1ce7512 (it should solve your issue).

@izimobil izimobil closed this Apr 15, 2021
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.

None yet

3 participants