Skip to content

Conversation

@yeaxi
Copy link
Contributor

@yeaxi yeaxi commented Jul 28, 2017

Fixes #9867
Refactor buildJarFileUrl();

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 28, 2017
Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

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

Thanks very much for opening your first Spring Boot pull request. I've left a couple of comments. Would you mind taking a look?

spec = spec.substring(0, spec.length() - SEPARATOR.length());
}
if (spec.indexOf(SEPARATOR) == -1) {
if (!spec.contains(SEPARATOR)) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please revert this part of the change as it's unrelated to the last modified time

return defaultTime;
}
try {
if (this.jarEntryName.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

This could be moved outside the try and handled in the if above:

if (this.jarFile == null || this.jarEntryName.isEmpty())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok)

@wilkinsona wilkinsona marked this as a duplicate of #9867 Jul 28, 2017
@wilkinsona wilkinsona added priority: normal type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 28, 2017
@wilkinsona wilkinsona added this to the 1.5.7 milestone Jul 28, 2017
@wilkinsona
Copy link
Member

Looks good now. Thank you, @yeaxi!

wilkinsona added a commit that referenced this pull request Jul 28, 2017
* gh-9893:
  Polish "Make JarURLConnection return entry's last modified time"
  Make JarURLConnection return entry's last modified time
@wilkinsona
Copy link
Member

Thanks again for the PR. This has been merged into 1.5.x and master.

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

Labels

type: bug A general bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants