Conversation
…rs; many minor bugfixes
…o tdl-from-grammar
…o tdl-from-grammar
goodmami
left a comment
goodmami
left a comment
There was a problem hiding this comment.
This is a huge PR so there are many comments. Some general things I noticed are:
- inconsistent use of
%,.format, and f-strings for formatting (sometimes causing issues which I've noted); but I get that this is updating old code so I didn't mention each of these - variables in Bash scripts should almost always be quoted when passed as arguments so that spaces do not break them into multiple arguments (I was getting a bit snarky toward the end because I was putting a comment on each time I saw this; sorry)
- some parts looked like the output of 2to3, which is fine, but the result isn't always idiomatic code. I generally did not comment on these, as there should be a separate commit to resolve style issues
I also did not attempt to run the code so I don't know if it works as a whole, but I'll leave that to you. The interfacing with PyDelphin's TDL module looked pretty good.
README.rst
Outdated
| sudo apt-get install python3-docutils python3-lxml | ||
|
|
||
| sudo pip install pydelphin --upgrade | ||
| sudo pip3 install pydelphin --upgrade |
There was a problem hiding this comment.
No need for both pip and pip3 if you're dropping support for Python 2. Also I don't suggest sudo for pip.
| * hyperlinked types | ||
| * types without glb | ||
|
|
||
|
|
There was a problem hiding this comment.
Personal todo lists don't serve much purpose in the repository. Actionable things can be made into issues, and personal notes can be left in unversioned files
|
|
||
|
|
||
| ## make a log in the same directory as the database | ||
| log = open(os.path.join(os.path.dirname(dbfile),"tdl.log"), 'w') |
There was a problem hiding this comment.
You've imported Path, so you could also do:
log = open(Path(dbfile).parent / 'tdl.log', 'w')| path = Path(grammarfile) | ||
| base = path.parent | ||
| for event, obj, lineno in tdl.iterparse(grammarfile): | ||
| if event == "LineComment": |
There was a problem hiding this comment.
Not necessary since the only other code block is if event == "EndEnvironment", so it's already filtered. But this makes sense if you plan to parse line comments later.
| orths = obj.conjunction.get('ORTH', default=None) | ||
| try: | ||
| orth=' '.join([str(s) for s in orths.values()]) | ||
| except: |
There was a problem hiding this comment.
Blanket exception. So you expect this to fail if orths is None? How about something like this (not tested):
if 'ORTH' in obj:
orths = ' '.join(map(str, obj['ORTH'].values()))
else:
orth = ''
print('No Orthography', obj.indentifier, sep='\t', file=log)
...If this generates an error, perhaps it's a bug in PyDelphin or some other case we need to handle separately.
make-ltdb.bash
Outdated
| LISP=" | ||
| ${extralisp} | ||
| (format t \"~%LTDB Read Grammar~%\") | ||
| LISP=""" |
There was a problem hiding this comment.
I don't think bash has triple-quoted (""") strings. This might work anyway if it's evaluated as an empty string "" followed by a multiline string.
|
Thanks.
…On Sun, Jun 28, 2020 at 10:42 PM Michael Wayne Goodman < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In make-ltdb.bash
<#13 (comment)>:
> @@ -161,11 +161,12 @@ then
unset DISPLAY;
unset LUI;
-LISP="
- ${extralisp}
- (format t \"~%LTDB Read Grammar~%\")
+LISP="""
I don't think bash has triple-quoted (""") strings. This might work anyway
if it's evaluated as an empty string "" followed by a multiline string.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#13 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIPZRSXTNHYCH5B3PRFBNTRY5JGFANCNFSM4N42YI7A>
.
--
Francis Bond <http://www3.ntu.edu.sg/home/fcbond/>
Division of Linguistics and Multilingual Studies
Nanyang Technological University
|
2 participants
@goodmami
could you please have a look at this? I would especially welcome some input on: