search

CLARIAH / COW

Integrated CSV to RDF converter, using CSVW and nanopublications
MIT License
46 stars 9 forks source link

String splitting of __str__ rendering of Resource (Item) object #97

Closed RinkeHoekstra closed 4 years ago

RinkeHoekstra commented 4 years ago

Really a bad idea. Just do a type check on the url_pattern to see if it's an instance of Resource (or Item if you want to be precise), and then take the _identifier property. Alternatively: override the __str__ method in the class definition of Item.

https://github.com/CLARIAH/COW/blob/83fc6de665a6104a92914899c2e887b8658736b0/src/converter/csvw.py#L750

melvinroest commented 4 years ago

In the 2020 version I took a simpler approach. It helps that it's purely python3, so it simplifies the function as well :)

My approach: check if it has the attribute of identifier (which is the official property that should be accessed, _identifier exists under the hood, if I recall correctly).

See:

  1. https://github.com/CLARIAH/COW/blob/2020/src/converter/csvw.py#L709 (the changed line)
  2. https://github.com/CLARIAH/COW/blob/2020/src/converter/util/__init__.py#L110 (how I'm checking for identifier)
RinkeHoekstra commented 4 years ago

Ah was not aware that there was a "2020" version that included refactoring. Perhaps good to call that out somewhere (and get rid of stale branches on GitHub?).

I still think it's safer to check the type than using hasattr.

melvinroest commented 4 years ago

Currently the first line in the most inner loop of process is an instantiation of Item. With that said, I agree that your suggestion is safer for maintainability reasons.