Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

#369: adding try/except to amendments getter #378

Merged
merged 2 commits into from
May 26, 2017

Conversation

gregoryfoster
Copy link
Contributor

Per @cmc333333's guidance, adding a try/except block around the amendments getter for the NoticeXML class. This enables bypassing of parsing errors in other CFR parts.

@cmc333333
Copy link
Member

Thanks @gregoryfoster! Looks like there are a few linter errors from Travis; would you mind addressing? Alternatively, there's a checkbox in the Github interface that allows maintainers to update your fork's branch, which would work.

Regarding the bare exception: do you know if there's a more specific exception being thrown? If not, we can just add # noqa to the line.

@gregoryfoster
Copy link
Contributor Author

Hi @cmc333333. I shrank the error message to address the line length warning and added # noqa to the bare except: line. At this time, I'm not sure what exceptions to expect, and feel like a catch-all is a good idea in this location. If there's a better exception handling pattern to follow, I'm opening up the branch for your commits and eager to learn!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 91.904% when pulling 9e91d00 on gregoryfoster:369-try-catch-amendments into b2a4c07 on eregs:master.

@cmc333333
Copy link
Member

Thanks @gregoryfoster! Unfortunately, we don't have a great exception-handling strategy at the moment (this is why you see so many non-helpful errors). I think, ideally, we'd have a grasp on what operations have a good chance of failing and account for those with human friendly error messages, but that's further off.

@cmc333333 cmc333333 merged commit 126c6e7 into eregs:master May 26, 2017
@gregoryfoster gregoryfoster deleted the 369-try-catch-amendments branch June 1, 2017 16:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants