Skip to content

Conversation

@kwin
Copy link
Member

@kwin kwin commented Mar 7, 2022

Provide alternatives relying on framework agnostic exception instead

@kwin kwin force-pushed the feature/deprecate-junit-assert-usage branch 2 times, most recently from 2133b8e to eb1d5e0 Compare March 7, 2022 14:15
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

some nits

@kwin kwin force-pushed the feature/deprecate-junit-assert-usage branch 2 times, most recently from 24c7c7e to c6a0216 Compare March 9, 2022 07:51
Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

It will be appreciated to add javadocs for new public methods.
Especially that you link new methods in deprecated ones.

@kwin kwin force-pushed the feature/deprecate-junit-assert-usage branch from c6a0216 to f535895 Compare March 10, 2022 07:39
@kwin kwin requested review from elharo and slawekjaranowski March 10, 2022 07:40
@kwin kwin force-pushed the feature/deprecate-junit-assert-usage branch from f535895 to 7403801 Compare March 10, 2022 07:42
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

I like this idea overall. I'm a little concerned that this is somewhat incompatible with existing usage. What other code outside this project uses the Verifier class?

@kwin kwin force-pushed the feature/deprecate-junit-assert-usage branch from 7403801 to 914b599 Compare March 10, 2022 18:29
@kwin
Copy link
Member Author

kwin commented Mar 10, 2022

I'm a little concerned that this is somewhat incompatible with existing usage.

This code should be fully backwards-compatible as no existing methods are removed/modified and only new ones are added. Some consumers can be seen at https://mvnrepository.com/artifact/org.apache.maven.shared/maven-verifier/usages. In any case a migration away from the deprecated methods should be pretty straightforward.

@elharo
Copy link
Contributor

elharo commented Mar 10, 2022

It's more of a long term concern. Is anyone who's not the Maven project using this? If so, it's going to be really hard to remove the deprecated methods.

I agree that if we had a green field this approach is preferable.

@slawekjaranowski
Copy link
Member

It is standard way, first deprecated method with documentation how should be replace - and we do this step.

Next we release major version with removed methods in some time.

@elharo
Copy link
Contributor

elharo commented Mar 11, 2022

It's the standard way, assuming step 2 can in fact be done. Far too often, step 2 never happens and the deprecated code is never removed and supported forever.

@elharo
Copy link
Contributor

elharo commented Mar 11, 2022

Thanks for the link to mvnrepository. That's more usages than I would have guessed. My gut is that we're not going to remove these methods. Too many projects depend on them already. I think we're better off living with what we have.

@slawekjaranowski
Copy link
Member

We shouldn't stop development based on simple usage.

Eg. now many project migrate to JUnit 5 or use other test frameworks than JUnit 4.
We shouldn't implicate which framework should be used by final project.

In my feel many projects do not upgrade their dependency, so it is their decision.

@slawekjaranowski
Copy link
Member

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

please use deprecated javadoc tag and annotation together.

@kwin kwin force-pushed the feature/deprecate-junit-assert-usage branch 2 times, most recently from fabe2a4 to 22a7de0 Compare March 11, 2022 16:09
@kwin kwin requested a review from slawekjaranowski March 11, 2022 19:47
@kwin kwin force-pushed the feature/deprecate-junit-assert-usage branch from 22a7de0 to 4bf4d80 Compare March 12, 2022 14:50
@slawekjaranowski
Copy link
Member

@kwin - what with open conversations with @elharo

Provide alternatives relying on framework agnostic exception instead
@kwin kwin force-pushed the feature/deprecate-junit-assert-usage branch from 4bf4d80 to ca75aff Compare March 13, 2022 10:21
@kwin
Copy link
Member Author

kwin commented Mar 13, 2022

what with open conversations with @elharo

I think everything should be resolved now.

@kwin kwin requested a review from elharo March 13, 2022 10:21
@kwin kwin requested a review from slawekjaranowski March 13, 2022 10:21
@slawekjaranowski slawekjaranowski self-assigned this Mar 13, 2022
@slawekjaranowski slawekjaranowski merged commit 0ae0517 into apache:master Mar 15, 2022
@slawekjaranowski slawekjaranowski added the deprecated Pull requests that deprecate features label Mar 17, 2022
@jira-importer
Copy link

Resolve #150

1 similar comment
@jira-importer
Copy link

Resolve #150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecated Pull requests that deprecate features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants