search

delph-in / pydelphin

Python libraries for DELPH-IN
https://pydelphin.readthedocs.io/
MIT License
79 stars 27 forks source link

fix issue #306 #309

Closed arademaker closed 4 years ago

arademaker commented 4 years ago
  1. solves issue #306
  2. sorry but https://github.com/delph-in/pydelphin/blob/develop/CONTRIBUTING.md is not clear about how to test. I don't know about tox and pip install -e .[test] didn't work for me. But I did test a few sentences and it seems to work.
  3. for now, I ended up including the empty sortinfo element, but I would prefer to remove it and make the case for updating the DTD.
goodmami commented 4 years ago

Sorry I've been a bit busy lately and just got around to this.

sorry but https://github.com/delph-in/pydelphin/blob/develop/CONTRIBUTING.md is not clear about how to test.

Good point. The CONTRIBUTING document assumes some familiarity with Python testing tools. I've opened #310 to track those issues.

pip install -e .[test] didn't work for me

This, however, is more surprising. You're on macOS, right? It might be a platform difference, as I'm on Linux.

for now, I ended up including the empty sortinfo element, but I would prefer to remove it and make the case for updating the DTD.

I think that's a good path. We should check with the group at Cambridge before changing the DTD. There are a few places needing revision, anyway (e.g., plus as an property value instead of + is just a DTD limitation). I had produced a RelaxNG schema some time ago. Maybe I can dig it up.

goodmami commented 4 years ago

Going by recent activity, I'm assuming you've had a chance to respond, so I'll merge as is, but I've changed the destination branch in preparation for a new release.

arademaker commented 4 years ago

Sorry @goodmami, I didn't have a chance to respond it before. I saw your comment about the Boolean test. Following the Lisp style, I would use if properties: ... but I saw in your code that many times you used the if properties is True: ... so I followed you.

Regarding the more inline version, I don't have any preference. I tend to prefer the version that makes the code more informative if that option doesn't affect performance.

arademaker commented 4 years ago

BTW, I am happy to be a contributor (tiny contribution I know) for this amazing library.

goodmami commented 4 years ago

but I saw in your code that many times you used the if properties is True: ... so I followed you.

Yes, I noticed that, too. It's good to keep things consistent. In those cases it was because other true/false values exist. E.g., for the indent level, indent=0 means to insert newlines but no spaces of indentation, where False means no newlines nor spaces. This is probably just bad design and maybe I can change it for a 2.0 release, but for now changing it would break backward compatibility.

Regarding the more inline version, I don't have any preference. I tend to prefer the version that makes the code more informative if that option doesn't affect performance.

Yes, in general either style is fine, but I switched to the inline style because the e.append(etree.Element('sortinfo', ...)) part was duplicated. Alternatively, this would be ok, too, as it reduces duplication:

if properties:
    sortinfo = node.sortinfo
else:
    sortinfo = {}
e.append(etree.Element('sortinfo', attrib=sortinfo))

BTW, I am happy to be a contributor (tiny contribution I know) for this amazing library.

Yes, thank you! I was hoping that this might break the ice for more contributions in the future :)