Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding area field to data #40

Closed
amanpalariya opened this issue Oct 2, 2020 · 8 comments · Fixed by #52
Closed

Adding area field to data #40

amanpalariya opened this issue Oct 2, 2020 · 8 comments · Fixed by #52
Assignees

Comments

@amanpalariya
Copy link
Contributor

The area is one of the very important stats for a country. So, it should be added to the data.

@bhatvikrant What should be the unit of area? I think km2 would be the best.

So, the schema will be

{
    "country": <country-name>
    ...
    "area": <area-in-sq-km>
}

Should we mention the unit in the data? 🤔

I guess it would be best to store area as a double and mention the unit in documentation only.

@bhatvikrant
Copy link
Owner

Yea, even I think km2 is the best option for the unit, it's widely used.

Don't mention the unit in the area key, instead make another key area_unit and put its value as km2.

what do you think? I'm open to suggestions @amanpalariya

@amanpalariya
Copy link
Contributor Author

I was thinking of not mentioning the unit in the data at all and just keep it limited to the documentation.

If we include area_unit, it will make the user think that some countries may have data in other units too.

@bhatvikrant
Copy link
Owner

bhatvikrant commented Oct 2, 2020

Hmm, interesting thought. @amanpalariya

I think we can tackle it by structuring the area schema as follows:

  ...
  "area": {
        value: 10000,
        unit: "sq. Km"            
   }
  ...

I think it's worth mentioning the unit in our API itself just for easier access and most times when using this library, developers may wanna show the unit too.

@sthiepaan
Copy link
Collaborator

sthiepaan commented Oct 3, 2020

If we don't want to deliver area unit then as @amanpalariya mentioned, we might use JSDocs (for methods) and README.md Documentation for that information.

If we include area_unit, it will make the user think that some countries may have data in other units too.

That actually could be a thing if we want to deliver english units such as square miles. But I think we should not focus on format/name of unit itself (let users handle this with their own i18n/translates/dictionaries).

The question is, do we want to deliver that for a users or should they calculate it by themselves? If we want to handle this it could be something like:

{
  // ...,
  "area": {
    "km2": 312679, // square kilometers
    "mi2": 120726 // square miles
  }
}

@amanpalariya
Copy link
Contributor Author

@sthiepaan Nice idea! 👍

This way, the area is provided in two major units.

But, if done this way, we will have to maintain consistency in data.json itself (can be done with the help tests only).

Extending @sthiepaan's idea, we can store area in km2 in data.json but while returning, we can convert the data into mi2 also and return what @sthiepaan proposed.

So, data.json contains

{
  //...,
  "area": 312769
}

But the method returns 👇

{
  //...,
  "area": {
    "km2": 312679,
    "mi2": 120726,
  }
}

@sthiepaan
Copy link
Collaborator

sthiepaan commented Oct 3, 2020

From my point of view, API should just return the date that it contain. Even if we have methods to grab specific data, we do not modify the output. If we create some functionality to convert the data, then we need to take care at least for correctness of calculated data and units rounding.

I would leave calculations for API consumers so they can decide what format they want to have. So just to summarize I think there are two proper ways:

  • Deliver data only in one format and let user play with that (I'd suggest using SI unit metre ➡️ m2)
  • Deliver mostly used units so users can just grab it for the API (so at least two areas units ➡️ km2 and mi2)

@bhatvikrant
Copy link
Owner

Yeah, @sthiepaan I agree. I think the unit conversions should be left to the consumer itself. Let's proceed with this

@amanpalariya
Copy link
Contributor Author

amanpalariya commented Oct 3, 2020

Since we are going with 👇

{
  //...,
  "area": {
    "km2": 312679,
    "mi2": 120726,
  }
}

So, we will have to create test to check that area in km2 equals area in mi2. I think I shall begin working on it now.

@sthiepaan sthiepaan linked a pull request Oct 18, 2020 that will close this issue
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 a pull request may close this issue.

3 participants