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

Upgraded to Junit v5.5 #250

Merged
merged 5 commits into from
Jul 15, 2019
Merged

Upgraded to Junit v5.5 #250

merged 5 commits into from
Jul 15, 2019

Conversation

Chaiavi
Copy link
Member

@Chaiavi Chaiavi commented Jul 14, 2019

Updated the annotations and assertions accordingly

This should close #249

Bringing the last 4 years into my fork
Updated the annotations and assertions accordingly
@Chaiavi
Copy link
Member Author

Chaiavi commented Jul 14, 2019

It passes when using Intellij Idea.
It fails commandline maven here:
Failed tests: crawlercommons.sitemaps.SiteMapURLTest.testSetPriority()

It didn't fail before my submit

@Chaiavi
Copy link
Member Author

Chaiavi commented Jul 14, 2019

Now all tests pass on Intellij Idea and also on commandline mvn

Please check out my pull request :-)

Fixing a styling issue I caused about 4 years ago

Details can be found here: #82
Copy link
Contributor

@sebastian-nagel sebastian-nagel left a comment

Choose a reason for hiding this comment

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

Thanks, @Chaiavi! Code builds and tests pass! Could you

  1. run mvn java-formatter:format to format the source code (see my comment how to preserve line breaks)
  2. squash the commits
    or let me know if I shall do it. Thanks!


sb.append("url = \"").append(url).append("\", lastMod = ").append((getLastModified() == null) ? "null" : SiteMap.W3C_FULLDATE_FORMATTER_UTC.format(getLastModified().toInstant()))
.append(", type = ").append(getType()).append(", processed = ").append(isProcessed()).append(", urlListSize = ").append(urlList.size());
StringBuilder sb = new StringBuilder()
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting is done by mvn java-formatter:format which would again put all the lines together. I agree (my personal preference) that every append in its own line is more readable. If // is added to every line the formatter will keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, nice hack, I will try it then run the mvn formatter command and check to see the results.

@@ -1,5 +1,5 @@
/**
* Copyright 2016 Crawler-Commons
* Copyright 2019 Crawler-Commons
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether we any policies regarding the year of the copyright notice. Afaik, the notices haven't been changed since added in 2016 (0775bb2).

Copy link
Member Author

Choose a reason for hiding this comment

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

I will revert the year changes,

If we decide to change the copyright year, we will do it in a separate commit dedicated to it (and done yearly)

Thanks again @sebastian-nagel

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's wait for comments or open a separate issue for it. And yes, if we decide to do it it should be done yearly (alternatively, in preparation for a release) and for all source files.

Fixing a styling issue I caused about 4 years ago

Details can be found here: #82
@Chaiavi
Copy link
Member Author

Chaiavi commented Jul 15, 2019

Ok, this commit fixes @sebastian-nagel code review

It passes the tests, and the formatter
Reverted back the year of the copyright

@Chaiavi Chaiavi merged commit 5bda363 into crawler-commons:master Jul 15, 2019
@Chaiavi
Copy link
Member Author

Chaiavi commented Jul 15, 2019

Oops, I just wanted to squash the commits but it also merged them into master, but I think it is a good commit

Should I revert?

@sebastian-nagel
Copy link
Contributor

Hi @Chaiavi, everything looks fine. Thanks! Reverting or anything which requires a forced push is rarely a good idea in an open source repository as it leads to errors for users which already fetched the affected commits.

@Chaiavi
Copy link
Member Author

Chaiavi commented Jul 16, 2019

Thankyou @sebastian-nagel

I will be wiser next squash and commit

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.

Upgrade Junit to v5.x
2 participants