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

Deprecate delegated check_* methods #75

Merged
merged 5 commits into from
Sep 25, 2023
Merged

Deprecate delegated check_* methods #75

merged 5 commits into from
Sep 25, 2023

Conversation

snacks02
Copy link
Contributor

I noticed these weren't private when playing with CrystalDoc.info.

This is indeed a breaking change, so the next release will have to be 2.0.0.

Vici37
Vici37 previously approved these changes Sep 25, 2023
Copy link
Collaborator

@Vici37 Vici37 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

This is a rather unremarkable reason to update a major version, though. As an alternative, what if instead we marked the methods with:

# :nodoc:
@[Deprecated("The visibility of this method will be changing in the future, and it should not be used directly")]
delegate check_jpeg, to: self.class

And then during a more noteworthy major release change, go through and clean up any @[Deprecated] methods that were identified? That way we avoid the trap of doing a major version update for every technically-breaks-backwards-compatibility bug fix 😅

What do you think?

@snacks02
Copy link
Contributor Author

snacks02 commented Sep 25, 2023

Apparently it doesn't work in the suggested order, but does work reversed.

image

require "./src/pluto"
require "./src/pluto/format/webp"

class Test
  include Pluto::Format::WebP
end

Test.new.check_webp(1)
> crystal test.cr
In test.cr:8:10

 8 | Test.new.check_webp(1)
              ^---------
Warning: Deprecated Test#check_webp. The visibility of this method will be changing in the future, and it should not be used directly

A total of 1 warnings were found.

@snacks02
Copy link
Contributor Author

Awesome suggestion, btw!

Copy link
Collaborator

@Vici37 Vici37 left a comment

Choose a reason for hiding this comment

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

🚀

@snacks02 snacks02 merged commit c50e379 into main Sep 25, 2023
2 checks passed
@snacks02 snacks02 changed the title Protect delegated check_* methods Deprecate delegated check_* methods Sep 25, 2023
@snacks02 snacks02 deleted the protect-check-methods branch November 22, 2023 05:23
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.

None yet

2 participants