Skip to content

Conversation

@wu-geoff
Copy link

Turning the latitude and longitude into protected variables, because there is no point making them public.

PS: It passed all the unit tests.

making the latitude and longitude into protected, because there is no point making them public
Copy link
Collaborator

@Kilo59 Kilo59 left a comment

Choose a reason for hiding this comment

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

I don't think this adds much value.
If you are just looking to get some open source commits under your built you could convert this model into a pydantic model and add some tests to verify the model passes/fails validation when appropriate values are passed.

I would approve and merge that.

As requested by Kilo59.

It still passed all the unit test, which is good. :)
@wu-geoff
Copy link
Author

wu-geoff commented Aug 3, 2021

I converted the Coordinates class to a pydantic model, and I specified the two fields (latitude and longitude) to be floats. Since NYT doesn't provide coordinates, the two fields can be set to None.
There are a couple things I must point out though:

  1. By specify the latitude and longitude to be float, they're no longer returned as strings in the responses. Instead, they're returned as floats, which might break backward compatibility.
  2. Also, since latitude and longitude are floats, if for some reasons the two fields are missing or just empty strings in the original data sets (e.g. in the JHU data set, the latitude and longitude of the Canadian Repatriated Travellers are both empty strings, for unknown reasons), they're now nulls in the JSON response, which, again, might break backward compatibility.

Let me know if you have any thoughts or opinions regarding my PR.

@ExpDev07 ExpDev07 closed this Aug 24, 2022
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