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

Few changes #5

Closed
wants to merge 5 commits into from
Closed

Few changes #5

wants to merge 5 commits into from

Conversation

aedobbyn
Copy link

Hi and thanks for the awesome package!

You can take or leave some or all of this PR. Thought I'd send it over since it's made what I'm trying to do with the package a bit easier. Main things are:

  • option to store key as an envir variable
  • a parse_forecast and parse_current in lowlevel.R that simplify the response and return similar-ish dataframes by default for easier comparison

Would love to hear your thoughts!

@aedobbyn
Copy link
Author

aedobbyn commented Aug 24, 2018

Oh and all the random spacing changes and stuff are from running styler::style_pkg() from styler so if you prefer a different style guide than their default the definitely feel free to disregard

@crazycapivara
Copy link
Owner

crazycapivara commented Aug 25, 2018

Hi, first of all thanx for your PR.

The style changes are fine for me, because it is the kind of style I also prefer, but it seems that I did not take care about it while coding owmr. Furthermore, I really like the feature storing the api key into an environment variable. So, for sure I will pick up this one.

I only had a quick look on your functions parse_current and parse_forecast. But in any case I would like to export them as separate functions and do not include them in the still somehow lowlevel functions get_current and get_forcast. In the beginning I also thought about some more parsing like renaming the list key to data or something with a clearer meaning but ended up by not doing this, so that everyone can write their parsing functions matching their use cases. That's why I also did not include the tidy_up functions into the main get_* funcs. With the piping operaror %>% it is easy to apply all kind of parsing functions to the data and it stays more flexible this way.

Moreover, due to changes in the functions a lot of tests now fail (devtools::test()) and there are also some more issues (like missing imports) making the build fail (devtools::check()).

So, one could just include/export the functions parse_current and parse_forecast or name them something like current_as_tibble, so that others could use them? I will take a closer look on them soon. What do you think about this approach?

@crazycapivara
Copy link
Owner

crazycapivara commented Aug 25, 2018

build issues

@aedobbyn
Copy link
Author

Yeah, I should have mentioned I didn't write any tests or make sure that the existing tests were passing since I wasn't sure if you'd be game for the idea :)

I totally get what you mean about leaving the get_current and get_forecast flexible. I think exporting current_as_tibble and forecast_as_tibble functions or going with the simplify boolean on get_current and get_forecast as is implemented here both work. Happy to do the former if you prefer.

I think the key thing is showing examples in the readme as "look at these nice dataframes you get with no effort" that are consistent (as much as possible) across functions. More attractive to people checking out the package than looking through the tidy_up functions to see which ones fit their use cases. (It's your package, of course, so feel free to disagree!)

By the way -- have you tested get_forecast_daily recently? I get a 401 (invalid api key).

library(owmr)
#> owmr 0.7.2
#>    another crazy way to talk to OpenWeatherMap's API
#>    Documentation: type ?owmr or https://crazycapivara.github.io/owmr/
#>    Issues, notes and bleeding edge: https://github.com/crazycapivara/owmr/
get_forecast(city = "New York", simplify = FALSE)$cod
#> [1] "200"
get_forecast_daily(city = "New York")$cod
#> [1] 401

@crazycapivara
Copy link
Owner

Hi, yep, it works for me, although the website says that the daily forecast is not included in the free plan :-)

@crazycapivara
Copy link
Owner

crazycapivara commented Aug 30, 2018

I added the option to store the api key in an environment variable called OWM_API_KEY.
Furthermore, I created a new branch feature/tibble where I put skeletons for current_as_tibble and forecast_as_tibble. Maybe you can do the implementation and add some tests as well!?
I think it would be nice to have a generic as_tibble function recognizing which kind of result needs to be parsed.

@aedobbyn
Copy link
Author

aedobbyn commented Sep 2, 2018

Hi, yep, it works for me, although the website says that the daily forecast is not included in the free plan :-)

Ahh makes sense.

Not super familiar with how you're doing the http mocking so I haven't yet been able to get a few of the tests to pass, but wanted to give you something to look at for as_tibble. Instead of creating classes for current and forecast responses, the approach just adds another element to the response list called timeframe which can either be current or forecast. as_tibble decides to use current_as_tibble or forecast_as_tibble depending on what timeframe is.

I'll open a PR for this change. Let me know what you think about that and I'll look further into the mocking and write some tests for as_tibble. Edit: this is the PR.

Re: the OWM_API_KEY -- looks good. Do you think we should keep reminding users to store their api key as an env variable even after they already have? The original PR had a mechanism for only suggesting that to users if the key was not found in an env file. Maybe you felt that was overcomplicating things?

@crazycapivara
Copy link
Owner

You are right, it does not make sense to remind users to store their api key in an environment variable in case they already do it. I will add your mechanism.

Furthermore, I will take a look at your new pull request as soon as possible. At the moment time is a little bit limited.

@aedobbyn
Copy link
Author

aedobbyn commented Sep 7, 2018

Awesome, no rush!

@crazycapivara
Copy link
Owner

Thanx again, I will close this one. Changes merged via #12.

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.

None yet

2 participants