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

remove ggplot2Animint dependency #23

Merged
merged 43 commits into from
Oct 8, 2018
Merged

remove ggplot2Animint dependency #23

merged 43 commits into from
Oct 8, 2018

Conversation

vivekktiwari
Copy link
Contributor

@tdhock test suite is passing.

sorry for extra PRs had to move all codes to merge branch

@tdhock
Copy link
Collaborator

tdhock commented Jun 15, 2018 via email

@vivekktiwari
Copy link
Contributor Author

@faizan-khan-iit Summary of changes made here:

  1. I have just merged the ggplot2Animint code from validate-params branch of yours, removed references to ggplot2Animint.
  2. Exported all Description and Namespace from ggplot2Animint, thanks for sharing your PRs that helped alot.
  3. I have also added data and data-raw files from ggplot2Animint code.
  4. There are major corrections in Examples and Documentation: ggplot2-ggproto.Rd is replaced with animint2-ggproto.Rd, also added all the things from there.

@tdhock
Copy link
Collaborator

tdhock commented Aug 10, 2018

looks good in general, glad to see all tests are passing

in this branch ggplot2/ggplot2Animint is no longer required, right? however on the README under Installation it still says to install ggplot2 -- can you please remove that line from the README? and maybe add a sentence or two that explains that ggplot2 functions have been forked and are included

@vivekktiwari
Copy link
Contributor Author

@tdhock removed the line, but I don't know why Travis is failing now. seems like error from Travis side.

@tdhock
Copy link
Collaborator

tdhock commented Oct 4, 2018

hi @vivekktiwari sorry it has taken me so long to look into this.

there was some problem with the test definition -- I had to add scale_y(breaks=something) in order to get the right number of grid lines to show up in the data viz -- they are now passing on my machine.

I also bumped the version and it looks like this is ready to merge with master right? (I am a bit confused about all of the other PRs you opened -- I assume this is the most relevant one -- please close the ones that are no longer relevant)

@vivekktiwari
Copy link
Contributor Author

hi @tdhock, sorry I have been inactive, all due to extra coursework, exams and placement,

Yes! This is the most relevant one. #25 is work-in-progress, I will resume working in a month, I will soon close irrelevant PRs.

@tdhock
Copy link
Collaborator

tdhock commented Oct 4, 2018 via email

@vivekktiwari
Copy link
Contributor Author

Is it feasible to separate the TEST_SUIT=CRAN from other test suits, as we won't be running major of the other test cases while submitting to CRAN, right?

@tdhock
Copy link
Collaborator

tdhock commented Oct 5, 2018 via email

@tdhock
Copy link
Collaborator

tdhock commented Oct 5, 2018

hey @vivekktiwari tests are now passing

can you please review the code and tell me if you think we should merge with master as it is right now, or do you think we need to add something else?

@vivekktiwari
Copy link
Contributor Author

@tdhock according to me, I think it is ready to merge.

README.org Outdated
@@ -18,6 +18,9 @@ new syntax.

** Differences with old animint


06-10-2018: =train_layout= [from ggplot2 - R/panel.R] function is now =g_train_layout=. =train_layout= [from animint2- R/z_facet.R] function is the same. Both are =internal functions=.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @vivekktiwari I moved this to NEWS since README should only contain info which is important for users (not info about internal functions)

@tdhock tdhock merged commit 1391eec into master Oct 8, 2018
@tdhock tdhock deleted the merge branch October 8, 2018 16:44
@tdhock
Copy link
Collaborator

tdhock commented Oct 8, 2018

merged! thanks so much for all your hard work @vivekktiwari

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.

2 participants