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

Ignore non file URLs when checking for directory or file extensions #83

Merged
merged 1 commit into from
Feb 23, 2020

Conversation

fp7
Copy link
Contributor

@fp7 fp7 commented Feb 21, 2020

In a URLClassLoader not all URLs have to be files or directories.
Eg. Spring boot's fat jars add entries like "jar:file:..." to the
classpath

@bbatsov
Copy link
Member

bbatsov commented Feb 21, 2020

The proposed change seems reasonable to me. Probably we can make the ifs a bit more compact, but that's not a big deal. Please, mention your fix in the changelog.

@fp7
Copy link
Contributor Author

fp7 commented Feb 21, 2020

How would you make it more compact?


(defn file-ext?
"Whether the argument's path ends in one of the specified case-insensitive
file extensions"
[f & exts]
(let [file (io/as-file f)]
(when-let [file (if (url? f)
(when (= (.getProtocol ^java.net.URL f) "file")
Copy link
Member

Choose a reason for hiding this comment

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

Here you can combine this condition with the previous using and (for instance). Or you can just replace the condition with one or/and.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still something to do here?

[f]
(.isDirectory (io/as-file f)))
(if (url? f)
(and (= (.getProtocol ^java.net.URL f) "file")
Copy link
Member

Choose a reason for hiding this comment

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

Here you can pull this and higher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like this ?

(and (url? f)
        (= (.getProtocol ^java.net.URL f) "file")
        (.isDirectory (io/as-file f)))

Then it would not check nonURLs correctly?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of something like:

(and (url? f) (= (.getProtocol ^java.net.URL f) "file")) ...

With a more complex conditional you can even get rid of the duplicated .isDirectory check, but this will probably make the code hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but i still do not understand.
You mean like this?

(defn directory?
  "Whether the argument is a directory or an url that points to a directory"
  [f]
  (if (and (url? f) (= (.getProtocol ^java.net.URL f) "file"))
    (.isDirectory (io/as-file f))))

Copy link
Member

Choose a reason for hiding this comment

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

My bad, I just realized where your confusion was coming from. If we structure the condition in this manner, the else branch won't be correct. I should stop reviewing code while doing something else. :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

## master (unreleased)

* [#83](https://github.com/clojure-emacs/orchard/pull/83): Ignore non file URLs when checking for directory or file extensions
Copy link
Member

Choose a reason for hiding this comment

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

You should also put "Bugs fixed" above this entry.

Copy link
Contributor Author

@fp7 fp7 Feb 21, 2020

Choose a reason for hiding this comment

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

Oh yeah, i see. Sorry

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

## master (unreleased)

* [#83](https://github.com/clojure-emacs/orchard/pull/83): Ignore non file URLs when checking for directory or file extensions
Copy link
Member

Choose a reason for hiding this comment

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

And a . at the end of this sentence. :-)

@bbatsov
Copy link
Member

bbatsov commented Feb 21, 2020

Can you also re-run the failed build? Seems like something unrelated, but still - let's see what happens if we run it a couple of times.

@fp7 fp7 force-pushed the master branch 2 times, most recently from 88fa60f to 1c51b36 Compare February 21, 2020 11:46
@fp7
Copy link
Contributor Author

fp7 commented Feb 21, 2020

Is there a button somewhere that i can click to rerun the tests?

In a URLClassLoader not all URLs have to be files or directories.
Eg. Spring boot's fat jars add entries like "jar:file:..." to the
classpath
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