Scan-o-Matic / scanomatic

Scanomatic
GNU General Public License v3.0
10 stars 4 forks source link

Refactor ScanningStore interface #292

Closed gdetrez closed 6 years ago

gdetrez commented 6 years ago

Remove duplication and simplify ScanningStore:

Remove get_free_scanners and corresponding API parameter.

Methods for more specific queries/updates still exists (e.g. get_current_job).

codecov-io commented 6 years ago

Codecov Report

Merging #292 into master will decrease coverage by 0.06%. The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
- Coverage   45.43%   45.37%   -0.07%     
==========================================
  Files         194      194              
  Lines       19150    19127      -23     
  Branches     2975     2972       -3     
==========================================
- Hits         8701     8679      -22     
- Misses      10019    10020       +1     
+ Partials      430      428       -2
Impacted Files Coverage Δ
scanomatic/ui_server/scanners_api.py 98.33% <100%> (ø) :arrow_up:
scanomatic/scanning/update_scanner_status.py 100% <100%> (ø) :arrow_up:
scanomatic/models/scan.py 100% <100%> (ø) :arrow_up:
scanomatic/ui_server/scans_api.py 96.66% <88.88%> (+0.05%) :arrow_up:
scanomatic/ui_server/scan_jobs_api.py 92.3% <90.9%> (+0.09%) :arrow_up:
scanomatic/io/scanning_store.py 97.36% <97.87%> (-0.71%) :arrow_down:
scanomatic/data_processing/phases/segmentation.py 73.18% <0%> (+0.27%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8875ccd...f8f2015. Read the comment docs.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.06%) to 42.066% when pulling f8f20154e1bf710548ced9088848b42e6bae8c0d on gdetrez:refactor-store into 8875ccddaae31d1d98d60023e6f4c56a9b34db5e on Scan-o-Matic:master.

gdetrez commented 6 years ago

The exists method has identifier as a keyword argument but when used it is always supplied as a positional argument. I don't know if such things are of any major concern but each time I cause them myself it kind of scratches a little "may I shouldn't be doing this" kind of feeling.

This allow to call exists in a similar way as both get and find.

gdetrez commented 6 years ago

Second thing isn't really much of a problem now, maybe none at all, but the way add method both check general validity of adding and has specific checks for different types. The latter feels somewhat misplaced especially if the number of checks started to grow. But I don't have a clear idea of how to do it differently either...

This is not ideal right now. The existing checks will ultimately be done by the db engine. Optionally, they could also be done in the domain logic too, e.g. if we want better error messages.

local-minimum commented 6 years ago

The exists method has identifier as a keyword argument but when used it is always supplied as a positional argument. I don't know if such things are of any major concern but each time I cause them myself it kind of scratches a little "may I shouldn't be doing this" kind of feeling.

This allow to call exists in a similar way as both get and find.

Not the find method, and if the find-method actually did do do it the exists could be reduced to

for _ in self.find(klass, id_, **constraints):
    return True
return False