Refactor postbox #69
Refactor postbox #69
Conversation
* Postbox fetching, data handling and download functionality have been rewritten in a more object-oriented fashion using data classes to improve readability and maintainability. It has been moved to a separate file.
* Corner cases like filenames without PDF extension, different source fields for dates, and documents without meaningful filenames are (hopefully) still fully supported.
* Prepend-date functionality has been fixed.
* JSON-API self-links are now used for mark-read updates.
* A new option has been added to store documents in per-account directories.
* A new CLI command ("download") with the new features has been added to keep the old CLI API stable.
* Test have been added.
|
Hi, refactoring the postbox part is on my list for quite some time. The current implementation was written in hurry, I never found the time to improve. Hence, your work is highly welcome. The current PR is marked as WIP feel free to continue and if you believe its ready for merge let me know. Again, thank you for your contribution. /G |
vanto
force-pushed
the
refactor-postbox
branch
from
09daad5 to
089670b
Compare
|
Awesome, thanks. I marked it WIP to have this discussion first. I think it is ready and can be merged. |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
Hi, I had a deeper look and really like your approach. Now my backlog got bid longer as i am planning to do the same for accounts, transactions and standing orders. |
|
Glad you like it. I'm happy to help with the refactorings (but I also only have limited time). Did you start already? If not, I could try to start with the transactions part... |
|
I used the holiday season and started to refactor the transaction part. First attempt can be found in the devel branch especially in 923085c. If you are willing to spend time, please review and provide feedback. This would be much more appreciated as I am not a professional developer and somehow missing best-practice. |
|
I skimmed through it briefly. Looks good to me, I left a minor comment regarding logging. I would probably go one step further and create dataclasses for the return structure of the API and use them as return values. This gives users of dkb_robo as a library the ability to read more values (such as the merchant subcategory). I would then put the functionality of flattening and filtering the values into different formatter classes. |
|
Thank you for your feedback. Your idea to "dataclass" the API structure sounds interesting and gives me the chance to get familiär with the dataclass concept. Let me play with it a bid more. |
|
Just to keep you updated. dkb_robo v0.29 has been released with a new option to enable unfiltered mode |
While working on my recent patches, I found it difficult to navigate the code base and tried to refactor the postbox part to increase understandability and maintainability by introducing separation of concerns using data classes and seeing the postbox as a more or less independent bounded context. My goal was to keep the original data as long as possible (though slightly abstracting the JSON:API structure). I believe other contexts (e.g. transactions) could also benefit from this structure (as I'm particular interested in getting additional information like the transaction ID in the exports).
Of course, this changes the style of the code base, and the style should be consistent. Hence, the question: What do you think? Do you like this approach, and would you like the idea of refactoring and splitting the code base following this more object-oriented way?
This is a summary of the changes: