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

Aggregation of tests for different S3 methods #168

Merged
merged 2 commits into from
May 25, 2015

Conversation

RomanTsegelskyi
Copy link
Contributor

Different tests for various S3 methods to exercise more S3 class parameters

@coveralls
Copy link

Coverage Status

Coverage increased (+1.43%) to 55.44% when pulling 993d3e4 on RomanTsegelskyi:s3_tests into 38ecce9 on Rapporter:master.

daroczig added a commit that referenced this pull request May 25, 2015
Aggregation of tests for different S3 methods
@daroczig daroczig merged commit 7d37b20 into Rapporter:master May 25, 2015
@daroczig
Copy link
Member

Thanks for this! Also, I've just realized that although the most recent release deprecates pander.return, we are still using it in a bunch of places: https://github.com/Rapporter/pander/search?utf8=%E2%9C%93&q=%22pander.return%22+path%3AR&type=Code

And also some other variants, like pandoc.p.return etc. in pandoc.R. We should similarly deprecate those, but I am not yet sure if the _return is the best approach. @RomanTsegelskyi, what do you think? Maybe we should ditch this (separate function) approach and use an argument instead?

@RomanTsegelskyi
Copy link
Contributor Author

@daroczig IMHO it would be more intuitive to ditch the _return approach. Looking at similar packages ascii, xtable don't have any similar options at all, thus shifting the burden completely to the user. stargazer has an options of directly specifying the output file, but not of capturing output. Altogether this brought me to thinking about use case of _return. And I could only come up with testing purposes. In such case it feels safe to ditch it. What do you think?

@daroczig
Copy link
Member

It's a hard decision for me: acknowledging that I picked the wrong approach ~3 yrs ago :)

But as we had to ditch pander.return, it's time to rationalize the related code as well. So we need to release a new version (probably 0.6) early summer with bunch of Deprecated calls for the given functions, so that the users/downstream packages can accommodate this change, then we can ditch all return functions at the end of GSoC.

For this end, I'd kindly ask you to

  1. work on the test cases as discussed (to verify that when we release 0.6, we do not have any calls to return inside of the package)
  2. after then, remove all return fns in a separate branch
  3. release new version

Then we can get back to the other goals of GSoC as discussed.

@RomanTsegelskyi
Copy link
Contributor Author

@daroczig that sounds like a good plan. I just submitted some chunk of test I have been working yesterday and this morning, so now I will switch to the do the things you have listed

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