lubianat / pyorcidator

MIT License
9 stars 5 forks source link

Add data model for quickstatements #41

Closed cthoyt closed 2 years ago

cthoyt commented 2 years ago

Closes #39

This pull request replaces the string concatenation implementation of building up the QuickStatements string with an alternative that uses classes to store the data in a list. Each class knows how to serialize itself to quickstatements as a string. In the end, this is done.

@jvfe maybe you want to think about how to test this works properly?

jvfe commented 2 years ago

This is awesome! Will definitely help clean up the code a lot and keep it much more maintainable.

I'll think on how this can be tested, I'm not very familiar with pydantic so I'll have to take a look.

But thanks a lot!

cthoyt commented 2 years ago

Just think of pydantic as the same as dataclasses with automatic json serialization built in (but you won’t need that feature)

cthoyt commented 2 years ago

@jvfe about time we blackened the whole project ;)

jvfe commented 2 years ago

@jvfe about time we blackened the whole project ;)

For sure, I'll probably include black in the CI too, just to be safe. But that's for another PR.

lubianat commented 2 years ago

it is a leftover from early implementations, should be removed!

Em qua., 12 de out. de 2022 07:52, João Vitor @.***> escreveu:

@.**** commented on this pull request.

In src/pyorcidator/helper.py https://github.com/lubianat/pyorcidator/pull/41#discussion_r993302744:

@@ -137,16 +164,18 @@ def process_item( ref, qualifier_nested_dictionary=None, ):

  • FIXME this is unused, delete?

I agree with deleting it, don't fully understand it either. Any comments @lubianat https://github.com/lubianat?

— Reply to this email directly, view it on GitHub https://github.com/lubianat/pyorcidator/pull/41#discussion_r993302744, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4NC732P6YZVZHUGVDWDDTWC2J5DANCNFSM6AAAAAARCYMIUQ . You are receiving this because you were mentioned.Message ID: @.***>

lubianat commented 2 years ago

Is there anything left here @jvfe @cthoyt ? can we merge?

cthoyt commented 2 years ago

@lubiana testing required

lubianat commented 2 years ago

I guess basic testing would just compare the currently generated quickstatements for a sample json to whatever the new code outputs. More granular tests would be good, but perhaps not necessary?

cthoyt commented 2 years ago

Just simple stuff like when you have a given instance of the model that you get out the text you expected.

cthoyt commented 2 years ago

Date precision is now handled, this is ready for merge @lubianat @jvfe