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

tidyr version of the getEurostatRaw #5

Merged
merged 1 commit into from
Aug 4, 2014
Merged

Conversation

jhuovari
Copy link

@jhuovari jhuovari commented Aug 4, 2014

I just noticed extract_numeric in tidyr and couldn't resist. It's faster than original.

Other comments: I think header = F in read.table and then manually giving column names that are not legal column names is not good idea. Better change them in getEurostatRCV. And mayby change them to dates? Or at least option to change them to dates?

antagomir added a commit that referenced this pull request Aug 4, 2014
tidyr version of the getEurostatRaw
@antagomir antagomir merged commit 638a423 into rOpenGov:master Aug 4, 2014
@antagomir
Copy link
Member

Thanks! Merged.

I support the idea with the column names. Pull requests are welcome, or I could even grant you push access to this repo?

@MansMeg
Copy link

MansMeg commented Aug 4, 2014

One way we often develop is to have one responsible for each repo (owner) and every one else working in branches. When a branch is done and tested we merge it. This way not all commits needs to be merged directly.

Skickat från min iPhone

4 aug 2014 kl. 16:39 skrev Leo Lahti notifications@github.com:

Thanks! Merged.

I support the idea with the column names. Pull requests are welcome, or I could even grant you push access to this repo?


Reply to this email directly or view it on GitHub.

@antagomir
Copy link
Member

I am the main responsible person for this package and agree this is in general a good strategy. The eurostat package is now under active development and for the time being I am accepting additions directly to master just to keep things simple as many things are now unstable anyway. I am planning to move to brancing when we get the the first properly functioning and tested version ready, hopefully during August/September when the time allows.

@jhuovari
Copy link
Author

jhuovari commented Aug 6, 2014

Thanks for the push access. I support move to branching, at least, I would be more comfortable to make changes that could break something.

Talking about breaking, I can make the column name change. Thinking about that change, I also started thinking about what getEurostatRaw should return. Mayby it should return more "raw" data.frame, without removal of additional marks. Those marks do have meaning and they could be useful for someone (and for future versions). I at least, have used them to get just non-forecast values.

So getEurostatRaw would be just downloading and turning to data.frame. Everything else would be done later. What do you think?

@antagomir
Copy link
Member

Ok, I have now created the devel branch. Kindly push your updates there after testing.

Absolutely welcome to make the column name change, and modify getEurostatRaw and related functions as suggested.

I fixed all warnings from pkg build. The package builds now work, excluding the vignette (ping @muuankarski ). Before continuing, you should pull my latest updates. These also include renaming 'getEurostatRCV' into 'get_eurostat' (we need to think updating the names for other functions as well) and archiving the statfi-related functions away from R/ directory. I also added double @@ in your email since Roxygen requires that (otherwise roxygenization with inst/extras/document.R fails). In fact, I would suggest removing your email from the roxygen docs, just naming us all as authors, and providing the public louhos@googlegroups.com as the generic contact address. Recommended for all developers to join the list, I can add you as well, it would be useful to handle any potential emails through this public email list. I have now added you as co-author in all relevant documentation.

@jhuovari
Copy link
Author

I have push changes to devel branch:

  • renamed getEurostatRaw to get_eurostat_raw
  • made the column name change
  • modified get_eurostat accordingly and moved all manipulation there
  • removed author field from both files, as I don't think its needed as we are all listed package authors

get_eurostat is now much faster thanks to tidyr.

@antagomir
Copy link
Member

Thanks ! Seems to work and merged to master now.

Regarding the author field, I think it would be beneficial for the users to have a single contact address (email or website) provided in function man pages, ie. in practice through Roxygen author field. Because it is easy to pick it from function manpage and not necessarily everyone knows the 'citation' command or where the package website is located for contact info. Hence I suggest we add the author/maintainer field back to these files. Any comments?

@jhuovari
Copy link
Author

I had an understanding that the author field in a function documentation should be used if author(s) are different from package authors, but actually Writing R Extensions doesn't say anything about that.
For me it's fine to be there. But should there rather be an url to the github page?

@antagomir
Copy link
Member

Moved this discussion to issue #7

antaldaniel added a commit that referenced this pull request Jun 19, 2021
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