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

Sample points uniformly from transformed space #2095

Merged
merged 6 commits into from
Oct 30, 2017

Conversation

thomasp85
Copy link
Member

Fixes #1992

@thomasp85 thomasp85 requested a review from hadley April 4, 2017 08:09
})

test_that("custom breaks works", {
vdiffr::expect_doppelganger(
Copy link
Member

Choose a reason for hiding this comment

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

This would be better as a regular test

Copy link
Member Author

Choose a reason for hiding this comment

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

All of them or just this one? Don't see how this one differs from the rest...

Copy link
Member

Choose a reason for hiding this comment

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

I think it's good to have one visual test to ensure the whole process basically works correctly. And then regular tests for everything else.

Copy link
Member Author

Choose a reason for hiding this comment

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

gotcha

"dup_axis",
ggplot(foo, aes(x, y)) +
geom_point() +
scale_x_continuous(name = "Unit A",
Copy link
Member

Choose a reason for hiding this comment

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

Better to set name to NULL here, since it's not germane to the test.

I'd also consider using geom_blank() to help focus on the important part of the visual test

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that's sloppy copy-pasting from my part

@hadley
Copy link
Member

hadley commented Jun 19, 2017

@thomasp85 are you interested in finishing this off? Otherwise @karawoo can take a look.

@thomasp85
Copy link
Member Author

Completely forgot about this one. Sure I'll finish it off👍

@hadley
Copy link
Member

hadley commented Jul 3, 2017

@thomasp85 still interested?

@thomasp85
Copy link
Member Author

Yep. Sorry, it seems this is constantly slipping out of focus... within the week

@thomasp85
Copy link
Member Author

Sorry for being slow on this - how is the new tests looking?

@hadley
Copy link
Member

hadley commented Jul 13, 2017

Looks good - but I think you need to delete a couple of the generated svgs?

@thomasp85
Copy link
Member Author

The fail on travis devel is related to stringr and not this PR

@hadley hadley merged commit 4825054 into tidyverse:master Oct 30, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants