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

Hard deprecate connect_to_etn() and reexport #313

Merged
merged 31 commits into from
Sep 30, 2024

Conversation

PietrH
Copy link
Member

@PietrH PietrH commented Sep 25, 2024

Previously on this dev branch, connect_to_etn() was no longer exported, and used internally as a helper. In this PR, a new helper is created to take over this work, and connect_to_etn() is re-exported, but hard deprecated: it always returns a lifecycle warning message, and it only returns NULL. All SQL helpers were adapted to use this new helper. Tests were changed over so both connect_to_etn() and the new helper are covered.

We decided to do it this way so older scripts that have con <- connect_to_etn() at the top are met with a deprecation message rather than a missing function.

connect_to_etn() is no longer neccesairy. Functions that need credentials will prompt the user for them.

I also added to NEWS and made a small change to a vignette.

@PietrH PietrH marked this pull request as ready for review September 25, 2024 15:17
@PietrH PietrH linked an issue Sep 25, 2024 that may be closed by this pull request
11 tasks
@PietrH PietrH requested a review from peterdesmet September 25, 2024 15:17
This was referenced Sep 26, 2024
@PietrH PietrH self-assigned this Sep 26, 2024
@PietrH
Copy link
Member Author

PietrH commented Sep 26, 2024

Ready for review

Copy link
Member

@peterdesmet peterdesmet left a comment

Choose a reason for hiding this comment

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

One comment. Would extend test with function call with parameters.

@PietrH PietrH requested a review from peterdesmet September 26, 2024 14:48
@PietrH
Copy link
Member Author

PietrH commented Sep 26, 2024

Made requested changes, ready for a second look if you have time. I'd appreciate your opinion on the changes to the vignette.

If not, let me know and we can merge.

Copy link
Member

@peterdesmet peterdesmet left a comment

Choose a reason for hiding this comment

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

I updated the vignette (please verify it's correct) and simplified the test to just make use of lifecycle::expect_deprecated()

@PietrH
Copy link
Member Author

PietrH commented Sep 27, 2024

I'm not a 100% sure how often you'll be prompted for them, but at least every time you click "Quit Session" so I think that's close enough.

Thanks for the review Peter!

@PietrH
Copy link
Member Author

PietrH commented Sep 27, 2024

The 2 tests in connect_to_etn() still fail:

Failure (test-connect_to_etn.R:5): connect_to_etn() returns deprecation warning when used
`connect_to_etn(username = "my name", password = "my password")` did not throw the expected warning.

Failure (test-connect_to_etn.R:8): connect_to_etn() returns deprecation warning when used
`connect_to_etn("my name", "my password")` did not throw the expected warning.

I'm looking into it, but would be grateful for any ideas!

Co-authored-by: Maëlle Salmon <maelle.salmon@yahoo.se>
@PietrH
Copy link
Member Author

PietrH commented Sep 30, 2024

I documented the behaviour of the tests and added skips for them, I'm now merging so this doesn't block the beta release.

I'm also planning on creating a minimal test package to see if I can replicate this behaviour, and if so, I'll go looking for help on https://forum.posit.co/

@PietrH PietrH merged commit c95988a into v2.3 Sep 30, 2024
@PietrH PietrH deleted the 303-deprecate-connect_to_etn-instead-of-not-exporting branch September 30, 2024 09:05
@PietrH
Copy link
Member Author

PietrH commented Sep 30, 2024

Thank you both for the constructive comments and contributions!

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.

deprecate connect_to_etn() instead of not exporting
3 participants