Runtime errors in run_filing are print statements

[richarddunks]

DESCRIPTION OF THE ISSUE

In run_filing of xml_runner.py, the final else clause, entered when there is a filing version not listed in ALLOWED_VERSIONSTRINGS or CSV_ALLOWED_VERSIONSTRINGS, has a print command not conditioned by the value of the verbose parameter, meaning the warning is printed when the OID is passed to run_filing and the Filing object instantiated. The Filing object is still returned by the function, so even though the version isn't allowed, the object is still instantiated and returned by the function but without the result attribute being set. Only then can the object be tested for its version.

This is true in run_sked where the final else statement contains the same code.

IMPACT

Print statements containing error information should be conditioned by the verbose flag for debugging purposes only so they can be suppressed in production code. Since this isn't passed as an exception, it can't be handled as an error when trying to access the Filling object with get_result and must be handled as a conditional on the Filing object's version_string. The possible workarounds include testing the first 4 characters for the version_string as an integer greater than 2012 or testing that Filing.result isn't None.

SUGGESTED CHANGES

1. Suppress the print statement if verbose = False in both run_filing and run_sked
2. Raise an exception when the filing version isn't supported for the operation either in get_result if there's a compelling reason to instantiate the Filing object when there is a filing version mismatch or in run_filing and run_sked if the Filing object shouldn't be instantiated when the filing version doesn't match the ALLOWED_VERSIONSTRINGS or CSV_ALLOWED_VERSIONSTRINGS
3. Make the code more DRY by combining the functionality of run_filing and run_sked so these functions don't repeat the same code.

JUSTIFICATION

Better error handling for this situation will help make this code work better without having to dive into the code internals to understand what versions are allowed and which aren't. Raising the exception allows the user to put this into a try-catch and figure out how they want to handle the program flow without having print statements that can't be suppressed. Consolidating the run_filing and run_sked functions will help improve the maintainability of this code going forward. Being able to process the schedules individually is a great feature but this could break if updates to one function aren't replicated in the other.

[jsfenfen]

Thanks so much for this detailed issue report.

You are absolutely right on all the code points.

This should probably be converted into a warning that only appears when verbose is chosen.

I may have occasion to address this in the next few months, this project has been pretty neglected.

I'm gonna add a longer list of years to suppress this.

FWIW the historical context is this: IRS used to regularly release the .xsd schema files; this library was built by parsing them. Of late these files have been harder and harder to come by, forcing folks to often run the library on schema versions where some xpaths (e.g. those added in the last tax year are unknown).

The process for updating the underlying metadata by hand or from .xsd files is pretty different, and it's not totally clear yet how IRS will land on this.

But suffice it to say the meaning of this warning has changed, and should be rethought.

[richarddunks]

Thanks for this. I'm happy to take care of #1. Exception handling isn't my forte as far as addressing #2 but there should be something for people to know there isn't anything in result, probably in the accessor function (`get_result`) so it fails properly, unless, like I said, returning an empty object is desirable behavior. #3 is more of a refactoring and I'm not sure how much of this you want done by someone outside the project, but I'm happy to help.

…

On Mon, Oct 25, 2021, 2:21 PM Jacob Fenton ***@***.***> wrote: Thanks so much for this detailed issue report. You are absolutely right on all the code points. This should probably be converted into a warning that only appears when verbose is chosen. I may have occasion to address this in the next few months, this project has been pretty neglected. I'm gonna add a longer list of years to suppress this. FWIW the historical context is this: IRS used to regularly release the .xsd schema files; this library was built by parsing them. Of late these files have been harder and harder to come by, forcing folks to often run the library on schema versions where some xpaths (e.g. those added in the last tax year are unknown). The process for updating the underlying metadata by hand or from .xsd files is pretty different, and it's not totally clear yet how IRS will land on this. But suffice it to say the meaning of this warning has changed, and should be rethought. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQPMYZOVXF4AKQJBR4X4ITUIWU4LANCNFSM5GV3U4TQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.