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

Fixes 104 81 152 #163

Merged
merged 6 commits into from
Aug 8, 2018
Merged

Fixes 104 81 152 #163

merged 6 commits into from
Aug 8, 2018

Conversation

mcmillanmajora
Copy link
Contributor

tdl.parse() now accepts file or filename
Fixes #104

tdl.parse_* functions are now private
Fixes #81

MRS dump() functions now allow files or filenames
Fixes #152

Tests were put in a separate file. They are now relocated to the file
with the other delphin.mrs.compare tests.
Functionality fixed in  `delphin.mrs.eds.dump()`,
`delphin.mrs.simplemrs.dump()`, `delphin.mrs.simpledmrs.dump()`,
`delphin.mrs.mrx.dump()`, `delphin.mrs.dmrx.dump()`,
`delphin.mrs.prolog.dump()`
Fixes #152
Copy link
Member

@goodmami goodmami left a comment

Choose a reason for hiding this comment

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

Nice work. My first comment (about the CHANGELOG) is an actual (albeit minor) problem, while the other two comments are just style issues. Clear these up and I'm ready to merge.

CHANGELOG.md Outdated
* `delphin.mrs.components.stringpred()`; replaced with
`delphin.mrs.components.surface()`
* `delphin.mrs.components.grammarpred()`; replaced with
`delphin.mrs.components.abstract()`
Copy link
Member

Choose a reason for hiding this comment

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

The paths are wrong; they are missing the Pred class before the method names

print(text, file=fh)
else:
with open(fh, 'w') as fh:
print(text, file=fh)
Copy link
Member

Choose a reason for hiding this comment

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

fh stands for "file handle", and if it doesn't have the write attribute it's not a file handle, so I would prefer that the incoming function parameter is renamed (e.g., destination or something), here and for the other serializers. E.g.,

else:
    with open(destination, 'w') as fh:
        print(text, file=fh)

In general I like to avoid assigning values of different types (e.g., string or file object here) to the same variable name. Even though it's valid Python, it gets confusing.

single=single,
properties=properties,
pretty_print=pretty_print,
**kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is odd here. if ms is on the same line as the function name, then the other parameters should line up with ms and not with the opening parenthesis. See PEP8.

    text = dumps(ms,
                 single=single,
                 properties=properties,
                 pretty_print=pretty_print,
                 **kwargs)

Or they can all be indented one level after the first line:

    text = dumps(
        ms,
        single=single,
        properties=properties,
        pretty_print=pretty_print,
        **kwargs)

CHANGELOG had incorrect method paths for `delphin.mrs.components.Pred`
update

Variables in mrs dump() functions renamed from fh to destination for
clarity; indentation fixed
Copy link
Member

@goodmami goodmami left a comment

Choose a reason for hiding this comment

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

Looks good. The change from fh to destination pushed some function signatures past 80 characters, but I'll accept the PR and try to do a project-wide PEP-8 compliance check as a separate issue.

Thanks!

@goodmami goodmami merged commit ee920de into develop Aug 8, 2018
@goodmami goodmami deleted the fixes-104-81-152 branch August 8, 2018 21:47
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.

2 participants