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

issue #480 add invisible(x) to print.teal_slice #636

Conversation

jimrothstein
Copy link
Contributor

Pull Request

Fixes #480
(First commit: want to be sure all is OK - Thx.)

…(My first commit; want to be sure everything is OK)
Copy link
Contributor

github-actions bot commented Jan 16, 2025

✅ All contributors have signed the CLA
Posted by the CLA Assistant Lite bot.

@jimrothstein jimrothstein marked this pull request as draft January 16, 2025 06:32
@jimrothstein
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@jimrothstein jimrothstein marked this pull request as ready for review January 16, 2025 06:54
Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Hi! Many thanks for your contribution!
This is a great start.

Could you please update print.teal_slices with a similar change?

Could you add tests for these changes on the corresponding test-teal_slice.R and test-teal_slices.R? Then we will be sure this remains working and fits the documentation.
Test could be something like teal_slice <- print(teal_slice(...)) and checking that the output is the same as the input.

@jimrothstein
Copy link
Contributor Author

@llrs-roche Thanks for guidance! I'm coming from admiral and everyone does PR a little different. Plus I'm novice with TEAL

So comments, suggestions .... needed and very welcome!

Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Yeah, each team have their preferences and methods. I'm sure I would also learn a lot if I contributed or used {admiral}.

You are on a great start with {teal}! I left two suggestions to make a test match the context and purpose of the file.

tests/testthat/test-teal_slice.R Outdated Show resolved Hide resolved
tests/testthat/test-teal_slice.R Outdated Show resolved Hide resolved
Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

All good now! Apologies for not raising this before, but could you add a NEWs entry explaining this? If not I'll do it myself before merging this.

@jimrothstein
Copy link
Contributor Author

@llrs-roche
Updated NEWS.md - hope ready to commit!

@llrs-roche llrs-roche enabled auto-merge (squash) January 21, 2025 07:49
@llrs-roche llrs-roche merged commit 3832ddb into insightsengineering:main Jan 21, 2025
26 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2025
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.

add return value to print methods
2 participants