incf-nidash / PyNIDM

Other
21 stars 31 forks source link

Replace "type(x) is y" with "isinstance(x, y)" #386

Closed yarikoptic closed 10 months ago

yarikoptic commented 12 months ago

Since type doesn't account for subclass inheritance at all, and likely in all those cases what matters is that an instance of that top level class.

Spotted while having a pick at https://github.com/incf-nidash/PyNIDM/pull/385/files

dbkeator commented 11 months ago

Sounds good. Thought I'd addressed all of those from the linters or one of the other pre-commits. I am going to check the test cases that failed in this pull request. For my previous pull request pytest came back clean....

yarikoptic commented 11 months ago

branch rf-typecheck-troubleshoot has troubleshooting helpers... it boiled down to following change

@@ -588,13 +589,13 @@ class Core:
             # context[key]['@id']= value.uri

             # context[key]['@type']='@id'
-            if type(value.uri) is str:
+            if isinstance(value.uri, str):
                 context[key] = value.uri
...
-            elif type(key) is URIRef:
+            elif isinstance(key, URIRef):
                 continue
             else:

because here

difference: o http://purl.org/nidash/nidm# is of type <class 'rdflib.namespace.Namespace'>, t is <class 'str'>, isinstance(o, t): True

so we have instance of Namespace which is subclass of str hence changing from type identity check to isinstance causes difference in behavior. Interestingly upgrade of pyld to 2.x series makes difference to go away!

note also that

In [4]: isinstance(URIRef('123'), str)
Out[4]: True

so that 2nd if is pretty much defunct -- but I guess we might not be hitting it.... need to figure out what is the intended logic here really ;) feels like we could for now RF just to continue for URIRef but then for anything else get its str

codecov-commenter commented 11 months ago

Codecov Report

Merging #386 (9dcd445) into master (29d653e) will decrease coverage by 0.01%. The diff coverage is 25.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
- Coverage   34.37%   34.37%   -0.01%     
==========================================
  Files          52       52              
  Lines        6845     6843       -2     
  Branches     1675     1674       -1     
==========================================
- Hits         2353     2352       -1     
  Misses       4322     4322              
+ Partials      170      169       -1     
Files Coverage Δ
src/nidm/experiment/Query.py 72.00% <100.00%> (ø)
src/nidm/experiment/Navigate.py 84.30% <0.00%> (ø)
src/nidm/experiment/tools/rest_statistics.py 0.00% <0.00%> (ø)
src/nidm/experiment/Core.py 51.06% <20.00%> (+0.01%) :arrow_up:
src/nidm/experiment/tools/bidsmri2nidm.py 0.00% <0.00%> (ø)
src/nidm/experiment/tools/rest.py 70.13% <38.46%> (ø)

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