Implemented Processor class and refactored some preliminary processing
#106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
QuanMPhm
force-pushed
the
99/implement_processor
branch
2 times, most recently
from
d72d59b
to
d9b838c
Compare
naved001
reviewed
Sep 26, 2024
QuanMPhm
force-pushed
the
99/implement_processor
branch
from
d9b838c
to
c8583a6
Compare
naved001
reviewed
Oct 7, 2024
process_report/process_report.py
Outdated
| lenovo_inv = lenovo_invoice.LenovoInvoice( | ||
| name=args.Lenovo_file, invoice_month=invoice_month, data=merged_dataframe.copy() | ||
| name=args.Lenovo_file, invoice_month=invoice_month, data=add_institute_proc.data |
There was a problem hiding this comment.
why are we not passing a copy of the dataframe here anymore?
There was a problem hiding this comment.
That's an honest mistake. Good catch!
process_report/process_report.py
Outdated
| ) | ||
| nonbillable_inv = nonbillable_invoice.NonbillableInvoice( | ||
| name=args.nonbillable_file, | ||
| invoice_month=invoice_month, | ||
| data=merged_dataframe.copy(), | ||
| data=add_institute_proc.data, |
QuanMPhm
force-pushed
the
99/implement_processor
branch
from
c8583a6
to
e6ab65b
Compare
naved001
approved these changes
Oct 10, 2024
knikolla
approved these changes
Oct 17, 2024
process_report/process_report.py
Outdated
| ) | ||
| nonbillable_inv = nonbillable_invoice.NonbillableInvoice( | ||
| name=args.nonbillable_file, | ||
| invoice_month=invoice_month, | ||
| data=merged_dataframe.copy(), | ||
| data=add_institute_proc.data.copy(), |
There was a problem hiding this comment.
It would be better if, after you are done with processing (for example on line 217), you assign the dataframe of the last Processor to a variable with a meaningful name like processed_data.
Passing over add_institute_proc.data requires
- The continued existence of the
add_institute_procobject instance so that you can access its dataframe - Knowing that the reason why you are passing that specifically here is because
add_institute_procis the last processor and therefore that is the fully processed dataframe, which requires you to scroll up to figure out that is the case. - In the case of adding another Processor, you need to change all instances of
add_institute_proc.datato their actual new processor name.
|
|
||
| @dataclass | ||
| class ValidatePIAliasProcessor(processor.Processor): | ||
| alias_map: dict |
There was a problem hiding this comment.
There's inconsistency in how you're initializing the 2 processors.
- ValidatePIAliasProcessor takes in a dictionary which contains the alias_map, whereas
- AddInstitutionProcessor loads its own file to get at the institution map.
I don't think you should change this logic now, but this is something that you should be aware when you implement further processors.
A `Processor` class has been added, subclassing from the `Invoice` class. This is the first step to refactor invoicing in order to seperate the processing and filtering/exporting functionalities of our current Invoice subclasses. Subclasses of `Processor` should only process invoices and manipulate its internal data, while subclasses of `Invoice` should only perform filtering and exporting, never changing any data itself. In addition to implementing `Processor`, two of its subclasses, `ValidatePIAliasProcessor` and `AddInstitutionProcessor` has been added to perform some preliminary processing steps.
QuanMPhm
force-pushed
the
99/implement_processor
branch
from
e6ab65b
to
8d56a6d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #99. Somewhat dependant on #100. A
Processorclass has been added, subclassing from theInvoiceclass. This is the first step to refactor invoicing in order to seperate the processing and filtering/exporting functionalities of our currentInvoicesubclasses. Subclasses ofProcessorshould only process invoices and manipulate its internal data, while subclasses ofInvoiceshould only perform filtering and exporting, never changing any data itself.In addition to implementing
Processor, two of its subclasses,ValidatePIAliasProcessorandAddInstitutionProcessorhas been added to perform some preliminary processing steps.