Initial refactoring of process_report #54
Merged
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
larsks
approved these changes
May 24, 2024
process_report/util.py
Outdated
| institution_name = institute_map.get(institution_key, "") | ||
|
|
||
| if institution_name == "": | ||
| print(f"Warning: PI name {pi_uname} does not match any institution!") |
There was a problem hiding this comment.
I would encourage the use of the logging module for emitting error and warning messages.
There was a problem hiding this comment.
@knikolla Should this become its own PR, or do I make use of logging in this intial refactor PR?
There was a problem hiding this comment.
@QuanMPhm For the specific invoices that you are refactoring in this PR, make use of logging.
knikolla
requested changes
May 24, 2024
|
|
||
| @dataclass | ||
| class NonbillableInvoice(invoice.Invoice): | ||
| """""" |
There was a problem hiding this comment.
Either write a docstring for the class or remove the double quotes.
This refactor commit is the first of a few, to lay out intial structure A new submodule, `invoices`, is added, containing a base class `Invoice` which is inherited by all other invoices. Currently, only the lenovo and nonbillable invoice has classes which inherits from `Invoice` Also created and partially populated an `util.py` file, containing functions placed above `main()` in `process_report.py` After the refactoring process is fully complete, these utility functions will be completely removed from `process_report.py`
QuanMPhm
force-pushed
the
52.1/initial_refactor
branch
from
3a82feb to
02c84ed
Compare
naved001
reviewed
Jun 17, 2024
naved001
approved these changes
Jun 18, 2024
Closed
22 tasks
knikolla
approved these changes
Jul 10, 2024
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 #63 . This refactor commit is the first of a few, to lay out initial structure
A new submodule,
invoices, is added, containing a base classInvoicewhich is inherited by all other invoices. Currently, only the lenovo and nonbillable invoice has classes which inherits fromInvoiceAlso created and partially populated anutil.pyfile, containing functions placed abovemain()inprocess_report.pyAfter the refactoring process is fully complete,these utility functions will be completely removed from
process_report.pyI can already tell I am breaking several Python conventions, so I am ready.