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

Project housecleaning #5

Merged
merged 13 commits into from
Aug 24, 2020
Merged

Conversation

runeflobakk
Copy link
Contributor

Some housecleaning chores to bring the code base and pom.xml up to speed. (duplicate of unruly/java-8-matchers#23, adapted for this fork)

  • upgade dependencies: hamcrest 2.2 and junit 5 (test)
  • upgrade Maven plugins
  • pom.xml restructure (see 4aa6434 for rationale behind changes)
  • enable a reasonable doclint for javadoc, to catch errors
  • skip building test-jar (I don't think anyone is depending on test code from this project?)
  • minor formatting fixups, and add .editorconfig (supported out of the box in IntelliJ and numerous other editors, some editors need a plugin to be aware of it, but not mandatory)

@runeflobakk
Copy link
Contributor Author

runeflobakk commented Jul 27, 2020

unruly/java-8-matchers#22, in particular unruly/java-8-matchers@caa0547, should fix the build on Java 9 and 11. I will move that PR to this repository after this one is merged.

@mrwilson
Copy link
Owner

Thanks for this! I'll pull the branch down and take a look tonight 👍

@@ -360,11 +360,11 @@ public void describeTo(Description description) {
/**
* The BaseStream must produce exactly the given expected items in order, and no more.
*
* For infinite BaseStreams see {@link #startsWith(T...)} or a primitive stream variant
* For infinite BaseStreams see {@link #startsWith(Object...)} or a primitive stream variant
Copy link
Owner

Choose a reason for hiding this comment

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

Is this an automatic fix suggested by doclint? Since it specifies @type <T> The type of items I'd expect it to be okay with the generic rather than Object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not an automatic fix, but seems to be the correct way of referencing a generically typed vararg parameter. Using #startsWith(T...) generates the following error from the javadoc-tool:

Error while generating Javadoc:
Exit code: 1 - StreamMatchers.java:363: error: reference not found
      * For infinite BaseStreams see {@link #startsWith(T...)} or a primitive stream variant
                                            ^
StreamMatchers.java:373: warning - Tag @link: can't find startsWith(T...) in uk.co.probablyfine.matchers.StreamMatchers

<artifactId>hamcrest-core</artifactId>
<version>1.3</version>
<artifactId>hamcrest</artifactId>
<version>2.2</version>
Copy link
Owner

Choose a reason for hiding this comment

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

I'm happy to bump up JUnit to 5, but I think bumping hamcrest from 1.x to 2.x is backwards incompatible.

So this is probably a major version bump for this library if this change makes it in. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, strictly speaking, yes. Though the only parts of Hamcrest used by java-8-matchers are:

  • org.hamcrest.Matchers
  • org.hamcrest.Description
  • org.hamcrest.TypeSafeDiagnosingMatcher
  • org.hamcrest.TypeSafeMatcher

Screenshot 2020-07-28 at 22 43 00

These classes are already in Hamcrest 1.3, and the APIs of these have not changed in 2.2. The upgrade to Hamcrest 2.2 did not result in any changes in src/main/java. If anybody uses java-8-matchers with Hamcrest 1.3 (typically depends directly on Hamcrest 1.3 as well as java-8-matchers), they will work together.

Though there are no mechanisms to tell us if backwards compatibility is broken at a later time, with changes in the library down the road. So the most correct thing may be to bump the major version. But merging this PR will actually not introduce any incompatibility with Hamcrest 1.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Myself, I really don't have any preference on bumping major version or not, as this is a type of library which is probably mostly, or maybe even always, used as a direct test-dependency, and not pulled in as a transitive dependency of something else. So upgrading java-8-matchers is just bumping the version from 1.x to 2, and I don't see the potential of any major version convergence conflicts with other dependencies which themselves may depend on 1.x of java-8-matchers.

As I see it, there should be no big deal to bumping the major version for a release after merging this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

I'll probably make a major version bump then, so at least v1 = Hamcrest 1.x, v2 = Hamcrest 2.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about merging #6 and #7 first to create a 1.9 release?

- missing whitespace
- star-imports
The doclint is set to "all except missing tags". It's a reasonable
setting to avoid having to add redundant noise for self-explanatory
paramters, types and such.
Use strictly pluginManagement for settings versions in order to be able
to invoke single plugins and get the same version as specified in the
build, as well as configuration.

Executions, in addition to plugins part of the default (jar) lifecycle,
are configured in build/plugins.

Configure versions-maven-plugin to be able to check for newer
dependencies/plugin versions

Add maven-enforcer-plugin to set minimal Maven version required by
configured plugins.
Change tests to being package scoped, and remove unnecessary exception
declarations.
maven-dependency-plugin analyzes and failes the build if:
- code is using a dependency which has been pulled in transitively, but
not declared as a dependency. Code used is direct dependency, and must
be declared as such.
- a dependency has been declared, but the code is actually not using it.
Thus, is can be removed (unless it is a runtime dependency, but the
nature of this library makes this very unlikely).
Default build command on Travis is mvn test

Reformat .travis.yml in accordance with .editorconfig (2 spaces indent)
Travis' default behaviour is to first do mvn install without tests to
"install" dependencies, and then execute a normal build with tests.
This is to align with the build lifecycle defined by Travis, but it is
not necessary for a Maven build, which should do everything it needs by
executing the idiomatic mvn clean install.

This should even further speed up the build.
Instead of CoreMatchers.is(..)
@mrwilson
Copy link
Owner

mrwilson commented Aug 8, 2020

I've released 1.9 today with #6 and #7 included 👍

@runeflobakk
Copy link
Contributor Author

Great! 🚀

@mrwilson
Copy link
Owner

mrwilson commented Aug 8, 2020

The plan is to merge this and release as 2.0.0 later on this weekend.

@runeflobakk
Copy link
Contributor Author

@mrwilson About 2.0.0-release, please see #8 :)

@mrwilson
Copy link
Owner

Going to merge this, then work on #8 before releasing as v2

@mrwilson mrwilson merged commit c6f9889 into mrwilson:master Aug 24, 2020
@runeflobakk runeflobakk deleted the project-housecleaning branch September 28, 2020 19:55
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