Skip to content

Conversation

strengejacke
Copy link
Member

@strengejacke strengejacke commented Apr 8, 2025

According to git blame, these are deprecated for at least 4 months now.

@strengejacke
Copy link
Member Author

@etiennebacher Not sure, but I think after removing safe, we always wanted to error, right?

@etiennebacher
Copy link
Member

I'd have to check in more detail but 4 months is not that old for an argument deprecation. Also this would be a breaking change so it would be better to use a minor version bump rather than a patch version for this.

@strengejacke
Copy link
Member Author

Also this would be a breaking change so it would be better to use a minor version bump rather than a patch version for this.

Yes, but we can set the final version number just before release. For the devel versions, it's important to have at least an increase for the fourth place, to make sure easystats::install_latest() works.

@strengejacke
Copy link
Member Author

but 4 months is not that old for an argument deprecation

Yes, usually not. But we deprecated this with version 1, so we already had that "API" change, which would justify the clean-up ;-) But I leave it to you to keep the deprecation messages longer and remove the arguments later.

@etiennebacher etiennebacher mentioned this pull request Apr 22, 2025
@etiennebacher
Copy link
Member

etiennebacher commented Apr 23, 2025

I'd like to have at least 6 months of deprecation, i.e. wait until early July, before removing them, so putting this on draft and this won't be a blocker for a potential release next week (cf #615).

@etiennebacher Not sure, but I think after removing safe, we always wanted to error, right?

Note to self: don't forget to address this comment before merging.

@etiennebacher etiennebacher marked this pull request as draft April 23, 2025 10:08
@strengejacke

This comment was marked as off-topic.

@strengejacke

This comment was marked as off-topic.

@etiennebacher

This comment was marked as off-topic.

@etiennebacher

This comment was marked as off-topic.

@strengejacke

This comment was marked as off-topic.

@etiennebacher etiennebacher mentioned this pull request Jul 14, 2025
2 tasks
@etiennebacher
Copy link
Member

Not sure, but I think after removing safe, we always wanted to error, right?

I think so, it is also what was documented in the parameter description. To me, "safe" in this context means "Don't let users think they renamed something when they actually didn't". So I'm keeping the behavior as is.

@etiennebacher etiennebacher marked this pull request as ready for review July 16, 2025 13:07
@etiennebacher etiennebacher changed the title Remove deprecated arguments Remove deprecated arguments in data_rename() and data_rename() Jul 16, 2025
@etiennebacher etiennebacher changed the title Remove deprecated arguments in data_rename() and data_rename() Remove deprecated arguments in data_rename() and data_match() Jul 16, 2025
@etiennebacher etiennebacher merged commit 5632d4d into main Jul 16, 2025
25 checks passed
@etiennebacher etiennebacher deleted the remove_deprecated branch July 16, 2025 13:20
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