Switch to onecall API + Review

[KristjanESPERANTO]

• switch to onecall API 3.0 (this time I tested it with a new API key)
• remove node_modules directory from repository
• lint ts file
• add prettifier
• add scripts to package.json
• fix format

I admit that the PR has become very large and is probably a bit tricky to review. Let me know if you want me to break it down into smaller PRs! 🙂

[scottcl88]

@KristjanESPERANTO What is the purpose of switching to One Call API? Per the OpenWeather documentation here, only the One Call API is changing and requires a credit card. The Current Weather API is free and satisfies the requirements of this module. Are there improvements to using this API over the other? Are you thinking of adding functionality?

Although I see plenty of room for code refactoring and clean up, adding the lint and prettier did very little to help and added extra bulk. I'd suggest if we go that route, we might as well actually clean up some of the code.

Also, I do see the benefit to removing node_modules from git, but then we need to update the documentation with npm install directions.

[KristjanESPERANTO]

Are there improvements to using this API over the other?

Oh, sorry! I used the example with locationID from the README and the module didn't work. I wrongly assumed (as you have already pointed out) that the cause was the deactivation of API 2.5. That's why I prematurely started implementing the new One Call API. Now I see that the reason it didn't work was that locationID is built into the URL as an id. But there is no id in Current Weather API. Did locationID ever worked? With lat and lon instead of locationID the module works fine without my changes.

Are you thinking of adding functionality?

Not at the moment. The ideas usually only come when I use the module for a while and put my hands into the code.

adding the lint and prettier did very little to help and added extra bulk.

There was already a prettier config in the package.json, I just got it to work. In my opinion, the additional mass is justified because it ensures a uniform code style and also detects some programming issues. This also helps, at least for me, when refactoring.

I'd suggest if we go that route, we might as well actually clean up some of the code.

Sure. If locationID really doesn't work, removing it might be a first step.

but then we need to update the documentation with npm install directions

For developers yes, but not for users. There are no dependencies that need to be installed for running the module. There are only devDependencies, which are only relevant for developers. So we would have to add developer instructions.

What happens next?

I suggest that we leave this PR for now and do small PRs one at a time. This would be easier to review and discuss. What do you think?

[scottcl88]

Are there improvements to using this API over the other?

Oh, sorry! I used the example with locationID from the README and the module didn't work. I wrongly assumed (as you have already pointed out) that the cause was the deactivation of API 2.5. That's why I prematurely started implementing the new One Call API. Now I see that the reason it didn't work was that locationID is built into the URL as an id. But there is no id in Current Weather API. Did locationID ever worked? With lat and lon instead of locationID the module works fine without my changes.

Are you thinking of adding functionality?

Not at the moment. The ideas usually only come when I use the module for a while and put my hands into the code.

Oh I see, yes locationID was working but it does seem like they have deprecated that. We should update the documentation. I wouldn't mind adding One Call API functionality to the module such as being able to use locationID, daily weather, etc. But I definitely suggest we keep a free no-credit-card option for this module. And not saying you have to do it.

adding the lint and prettier did very little to help and added extra bulk.

There was already a prettier config in the package.json, I just got it to work. In my opinion, the additional mass is justified because it ensures a uniform code style and also detects some programming issues. This also helps, at least for me, when refactoring.

I'd suggest if we go that route, we might as well actually clean up some of the code.

Sure. If locationID really doesn't work, removing it might be a first step.

but then we need to update the documentation with npm install directions

For developers yes, but not for users. There are no dependencies that need to be installed for running the module. There are only devDependencies, which are only relevant for developers. So we would have to add developer instructions.

Good points, I overlooked that. Thank you. Adding lint and removing node_modules would certainly be good then.

What happens next?

I suggest that we leave this PR for now and do small PRs one at a time. This would be easier to review and discuss. What do you think?

Agreed. I appreciate the clarification and discussion, thanks.

[KristjanESPERANTO]

But I definitely suggest we keep a free no-credit-card option for this module.

Free is still possible with OpenWeather (1,000 API calls per day), but as far as I know not anymore without a credit card 🙁

[scottcl88]

@KristjanESPERANTO The "Current Weather" service is free without a credit card. You can create an account without adding billing details and with an Api key you can use that API. It's limited to 60 calls/minute and 1,000,000 calls/month. I just tested this again.

https://openweathermap.org/price
https://openweathermap.org/current

[KristjanESPERANTO]

Thanks for testing it out! I still get confused with the One Call API, sorry. So, there is no need to switch to another API for now.