Skip to content

Removed unused checked exceptions #2036

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

Merged
merged 3 commits into from
Feb 25, 2025
Merged

Conversation

rnveach
Copy link
Contributor

@rnveach rnveach commented Feb 17, 2025

Description

Removes exceptions not thrown by the methods.

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.57%. Comparing base (5d3344a) to head (15ca8ba).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2036   +/-   ##
=========================================
  Coverage     83.57%   83.57%           
  Complexity     2380     2380           
=========================================
  Files           235      235           
  Lines          7259     7259           
  Branches        382      382           
=========================================
  Hits           6067     6067           
  Misses          956      956           
  Partials        236      236           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bitwiseman
Copy link
Member

Removing exceptions... I thought this was a binary breaking change, but the API checks didn't fail for this change...

I need to look at this further.

@rnveach
Copy link
Contributor Author

rnveach commented Feb 18, 2025

Do you want me to try and split this between main and test?

Also, I would think this would still be acceptable to do to non-public methods in main as users can't write code to link to them outside of reflection anyways.

Finally, can you also provide more information on this project's stance on binary breaking change? Is this project fully against them, or this requires a special release?

Also what checks is used to catch these? I see the POM has japicmp-maven-plugin.

@rnveach
Copy link
Contributor Author

rnveach commented Feb 18, 2025

Documentation:
https://docs.oracle.com/javase/specs/jls/se11/html/jls-13.html#jls-13.4.21 (Java 11 JLS)

Changes to the throws clause of methods or constructors do not break compatibility with pre-existing binaries; these clauses are checked only at compile time.

This says it is not a breaking binary change.

This section also hasn't changed up to JLS 22 ( https://docs.oracle.com/javase/specs/jls/se22/html/jls-13.html#jls-13.4.21 ).

@bitwiseman bitwiseman self-requested a review February 24, 2025 18:17
Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

Updating with latest changes. Assuming japicmp reports all clear, we'll take this.

Thanks!

@bitwiseman bitwiseman changed the title removed exceptions not thrown Removed unused checked exceptions Feb 25, 2025
@bitwiseman bitwiseman merged commit 8343b6b into hub4j:main Feb 25, 2025
12 checks passed
@rnveach rnveach deleted the unused_exceptions branch February 26, 2025 00:39
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