Skip to content

[ENG-7445] Added IndexcardJsonDeriver#868

Merged
aaxelb merged 6 commits intoCenterForOpenScience:feature/shtrove-enhance-2025-05from
bodintsov:feature/shtrove-osfmap_json_mini
May 22, 2025
Merged

[ENG-7445] Added IndexcardJsonDeriver#868
aaxelb merged 6 commits intoCenterForOpenScience:feature/shtrove-enhance-2025-05from
bodintsov:feature/shtrove-osfmap_json_mini

Conversation

@bodintsov
Copy link
Contributor

@coveralls
Copy link

coveralls commented May 9, 2025

Coverage Status

coverage: 81.248% (+0.02%) from 81.233%
when pulling b37bb2b on bodintsov:feature/shtrove-osfmap_json_mini
into ea9e5ba on CenterForOpenScience:feature/shtrove-enhance-2025-05.


@staticmethod
def _should_keep_predicate(predicate: str) -> bool:
return True if predicate in PREDICATE_LIST else True
Copy link
Contributor

Choose a reason for hiding this comment

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

this always returns True

Copy link
Contributor

@aaxelb aaxelb left a comment

Choose a reason for hiding this comment

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

small requests and a rename; seems near ready

ns.DCTERMS.dateModified,
ns.OSFMAP.hostingInstitution,
ns.OSFMAP.keyword,
ns.OSFMAP.contains,
Copy link
Contributor

Choose a reason for hiding this comment

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

osf:contains is probably the property most worth excluding -- lists every osfguid-identified file in an osf proj/reg, which could be many (and could be helpful sometimes but we do not need that in search results)


@staticmethod
def _should_keep_predicate(predicate: str) -> bool:
return True if predicate in INCLUDED_PREDICATE_SET else False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return True if predicate in INCLUDED_PREDICATE_SET else False
return (predicate in INCLUDED_PREDICATE_SET)

nit: the ternary here overcomplicates (the parentheses aren't necessary but i think they help clarify intent for the hasty reader)

})


class IndexcardJsonDeriver(OsfmapJsonFullDeriver):
Copy link
Contributor

Choose a reason for hiding this comment

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

(was going to ignore this but then i got confused often enough while reading it seems worthwhile to) rename IndexcardJsonDeriver to OsfmapJsonMiniDeriver, to be consistent with the module and parent class naming

self.assertEqual(expected, json.loads(actual))


class TestIndexcardJsonDeriver(BaseIndexcardDeriverTest):
Copy link
Contributor

Choose a reason for hiding this comment

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

belongs in its own file test_osfmap_json_mini.py (aligning with the other test files in this dir and mirroring code file layout)

@aaxelb aaxelb merged commit eb3a993 into CenterForOpenScience:feature/shtrove-enhance-2025-05 May 22, 2025
3 checks passed
@aaxelb
Copy link
Contributor

aaxelb commented May 22, 2025

(oops meant to merge to develop -- have now done by hand)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants