Runtime errors in run_filing are print statements
Opened this issue · 2 comments
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
- Suppress the
print
statement ifverbose = False
in bothrun_filing
andrun_sked
- 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 inrun_filing
andrun_sked
if the Filing object shouldn't be instantiated when the filing version doesn't match theALLOWED_VERSIONSTRINGS
orCSV_ALLOWED_VERSIONSTRINGS
- Make the code more DRY by combining the functionality of
run_filing
andrun_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.
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.