search

HEPCloud / decisionengine

HEPCloud Decision Engine framework
Apache License 2.0
6 stars 26 forks source link

Use public .keys() rather than internal implementation #478

Closed jcpunk closed 3 years ago

jcpunk commented 3 years ago

If we need to alter how the .keys() function returns to maintain our public API, this will help keep us consistent.

If we stay with the public functions we can be assured we aren't doing anything sneaky internally.

codecov[bot] commented 3 years ago

Codecov Report

Merging #478 (fe6fd2b) into master (94c1411) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #478   +/-   ##
=======================================
  Coverage   93.52%   93.52%           
=======================================
  Files          43       43           
  Lines        2703     2703           
  Branches      384      384           
=======================================
  Hits         2528     2528           
  Misses        135      135           
  Partials       40       40
Flag Coverage Δ
python-3.6 92.87% <100.00%> (ø)
python-3.9 92.96% <100.00%> (ø)
python-pypy-3.7 93.24% <100.00%> (ø)

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

Impacted Files Coverage Δ
...rc/decisionengine/framework/dataspace/datablock.py 98.75% <100.00%> (ø)

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 94c1411...fe6fd2b. Read the comment docs.

jcpunk commented 3 years ago

Why the change to use the public method in the class itself?

Generally when a class has a public method to do a thing, I try to use it internally. That both helps ensure that changes to the public api are sufficiently painful.

This is sorta a matter of preference, but if we've got a public way to do a thing, re-implementing a different version privately feels weird and like unnecessary duplication.

If the last patch I sent Steve works out, I'll need to change out the internal implementation for how the list of known keys is determined. Having the code primed to make that a single place to change would be handy.

https://github.com/HEPCloud/decisionengine/pull/470/commits/44c636e913e93bc40ea60dcf4f1a634721a567e0

Thinking about it because .keys() use in python has been reduced and this change would anchor it more.

With this function already on our public api, I'm not sure we can really dispose of it.....