Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Stronger typing for file consumption #2744

Merged
merged 1 commit into from Apr 1, 2023

Conversation

stumpylog
Copy link
Member

@stumpylog stumpylog commented Feb 24, 2023

Proposed change

This is a larger change, but I think worthwhile to make. It creates 2 data classes for handling typing and serialization of types when calling the main shared task consume_file. Some of the benefits I can think of:

  • Arguments are consistently serialized and serialized into the correct types, instead of maybe becoming unexpected strings
  • File mime type is gotten once, early on, instead of multiple times during consume and stored for all needs downstream
  • New ability to define where the document is coming from, allowing some simplifications now (and maybe useful in the future)
  • Easier to add new overrides
  • Testing is less dependent on kwarg names, but can use strongly typed and named fields instead
  • Testing utility for getting last call or call by index, nicely decoded included

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other (please explain)

Checklist:

  • I have read & agree with the contributing guidelines.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • If applicable, I have checked that all tests pass, see documentation.
  • I have run all pre-commit hooks, see documentation.
  • I have made corresponding changes to the documentation as needed.
  • I have checked my modifications for any breaking changes.

@paperless-ngx-secretary paperless-ngx-secretary bot added backend non-trivial Requires approval by several team members labels Feb 24, 2023
@github-actions github-actions bot added the enhancement New feature label Feb 24, 2023
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #2744 (60c1c53) into dev (dde3205) will decrease coverage by 0.02%.
The diff coverage is 95.58%.

❗ Current head 60c1c53 differs from pull request most recent head d7cafbe. Consider uploading reports for the commit d7cafbe to get more accurate results

@@            Coverage Diff             @@
##              dev    #2744      +/-   ##
==========================================
- Coverage   93.22%   93.20%   -0.02%     
==========================================
  Files         154      152       -2     
  Lines        6497     6435      -62     
==========================================
- Hits         6057     5998      -59     
+ Misses        440      437       -3     
Flag Coverage Δ
backend 93.20% <95.58%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/documents/consumer.py 95.71% <ø> (ø)
src/paperless_mail/mail.py 88.00% <71.42%> (-0.52%) ⬇️
src/documents/tasks.py 95.30% <94.73%> (+2.99%) ⬆️
src/documents/barcodes.py 100.00% <100.00%> (ø)
src/documents/data_models.py 100.00% <100.00%> (ø)
...documents/management/commands/document_consumer.py 93.93% <100.00%> (-0.04%) ⬇️
src/documents/signals/handlers.py 88.28% <100.00%> (-0.29%) ⬇️
src/documents/views.py 95.64% <100.00%> (-0.01%) ⬇️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@stumpylog stumpylog force-pushed the feature-consume-doc-container branch 2 times, most recently from 8129ac1 to 009ce2f Compare March 1, 2023 22:57
src/documents/data_models.py Outdated Show resolved Hide resolved
src/documents/data_models.py Show resolved Hide resolved
src/documents/data_models.py Outdated Show resolved Hide resolved
src/documents/management/commands/document_consumer.py Outdated Show resolved Hide resolved
src/documents/tasks.py Outdated Show resolved Hide resolved
src/documents/data_models.py Outdated Show resolved Hide resolved
src/documents/tasks.py Show resolved Hide resolved
@stumpylog stumpylog force-pushed the feature-consume-doc-container branch from 009ce2f to 7c1ac05 Compare March 6, 2023 21:15
@shamoon shamoon added this to the v1.14 milestone Mar 7, 2023
@stumpylog stumpylog force-pushed the feature-consume-doc-container branch 4 times, most recently from fc2805a to 60c1c53 Compare March 13, 2023 05:27
@stumpylog stumpylog marked this pull request as ready for review March 15, 2023 14:27
@stumpylog stumpylog requested a review from a team as a code owner March 15, 2023 14:27
@stumpylog stumpylog force-pushed the feature-consume-doc-container branch from 60c1c53 to 92f055e Compare March 29, 2023 20:06
…typing of arguments and setting of some information about the file only once
@stumpylog stumpylog force-pushed the feature-consume-doc-container branch from 92f055e to d7cafbe Compare March 30, 2023 22:15
Copy link
Member

@shamoon shamoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All issues look resolved, thanks stumpy!

@stumpylog stumpylog merged commit 3c2bbf2 into dev Apr 1, 2023
29 checks passed
@stumpylog stumpylog deleted the feature-consume-doc-container branch April 1, 2023 18:05
@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backend enhancement New feature non-trivial Requires approval by several team members
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants