Skip to content

Eastwood: always use absolute filenames, internally #187

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Eastwood: always use absolute filenames, internally #187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion e2e/monorepo-support/test/monorepo_support/core_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

(doseq [filename all
:let [file (File. filename)
absolute (-> file .getAbsolutePath)]]
absolute (-> file .getCanonicalPath)]]
Copy link
Contributor Author

@vemv vemv Mar 30, 2021

Choose a reason for hiding this comment

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

Settling in using getCanonicalPath only seems a good idea:

  • one less thing to remember
  • importantly, it resolves symlinks, resulting in cache keys that can hit more often

From memory, libs like tools.namespace favor .getCanonicalPath

(is (= filename
absolute)
"Returns absolutized filenames, which is important for monorepo support")
Expand Down
25 changes: 19 additions & 6 deletions src/formatting_stack/linters/eastwood.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
(ns formatting-stack.linters.eastwood
(:require
[clojure.java.io :as io]
[clojure.set :as set]
[clojure.string :as str]
[eastwood.lint]
[formatting-stack.linters.eastwood.impl :as impl]
[formatting-stack.protocols.linter :as linter]
[formatting-stack.protocols.spec :as protocols.spec]
[formatting-stack.util :refer [ns-name-from-filename silence]]
[medley.core :refer [assoc-some deep-merge]]
[nedap.utils.modular.api :refer [implement]])
[nedap.speced.def :as speced]
[nedap.utils.modular.api :refer [implement]]
[nedap.utils.spec.api :refer [check!]])
(:import
(java.io File)))

Expand All @@ -16,12 +19,20 @@
(assoc :rethrow-exceptions? true)))

(defn lint! [{:keys [options]} filenames]
{:post [(do
Copy link
Contributor Author

@vemv vemv Mar 30, 2021

Choose a reason for hiding this comment

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

A glorified :such-that ;p

An alternative (or complementary) solution would be strengthening ::protocols.spec/filename from present-string? to a string that denotes an absolutized, existing filename.

IIRC a very similar idea was rejected ~1y ago (can't find the link, sorry) so I'm fine with leaving things as-is

Copy link
Member

@thumbnail thumbnail Mar 31, 2021

Choose a reason for hiding this comment

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

from present-string? to a string that denotes an absolutized, existing filename. IIRC a very similar idea was rejected ~1y ago

I guess the reasoning would be something akin to "don't have side-effecting specs". I.e. a absolute filepath is fine (i.e. leading /), going through the filesystem is not.

It makes generating examples harder and makes a spec unpure / dependent on state outside of it's input

(assert (check! (speced/fn [^::protocols.spec/reports xs]
(let [output (->> xs (keep :filename) (set))]
(set/subset? output (set filenames))))
%)
"The `:filename`s returned from Eastwood should be a subset of this function's `filenames`.
Otherwise, it would mean that our filename absolutization out of Eastwood reports is buggy.")
true)]}
(let [namespaces (->> filenames
(remove #(str/ends-with? % ".edn"))
(keep ns-name-from-filename))
reports (atom nil)
exceptions (atom nil)]

(silence
(try
(-> options
Expand All @@ -37,9 +48,11 @@
:level :warning
:source (keyword "eastwood" (name linter))
:warning-details-url warning-details-url
:filename (if (string? uri-or-file-name)
uri-or-file-name
(-> ^File uri-or-file-name .getCanonicalPath)))))
:filename (speced/let [^::speced/nilable ^String s (when (string? uri-or-file-name)
uri-or-file-name)
^File file (or (some-> s File.)
uri-or-file-name)]
(-> file .getCanonicalPath)))))
(into (impl/exceptions->reports @exceptions)))))

(defn new [{:keys [eastwood-options]
Expand Down
2 changes: 1 addition & 1 deletion test/formatting_stack/test_helpers.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
(speced/defn filename-as-resource [^string? filename]
(str "file:" (-> filename
File.
.getAbsolutePath)))
.getCanonicalPath)))

(defn with-mocked-diff-path
"Fixture to stub the absolute path in #'util.diff/unified-diff"
Expand Down
2 changes: 1 addition & 1 deletion test/functional/formatting_stack/formatters/clean_ns/impl.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
(deftest has-duplicate-requires?
(are [input assertion] (let [file (io/file "test" "functional" "formatting_stack" "formatters" "clean_ns" input)
_ (assert (-> file .exists))
filename (-> file .getAbsolutePath)
filename (-> file .getCanonicalPath)
result (sut/has-duplicate-requires? filename)]
(case assertion
:has-duplicates result
Expand Down
12 changes: 7 additions & 5 deletions test/functional/formatting_stack/linters/eastwood.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
[formatting-stack.linters.eastwood :as sut]
[formatting-stack.protocols.linter :as linter]
[matcher-combinators.matchers :as matchers]
[matcher-combinators.test :refer [match?]]))
[matcher-combinators.test :refer [match?]])
(:import
(java.io File)))

(use-fixtures :once (fn [tests]
;; prevent humongous AST representations from being printed:
Expand All @@ -14,7 +16,7 @@
(deftest lint!
(let [linter (sut/new {})]
(are [filename expected] (match? expected
(linter/lint! linter [filename]))
(linter/lint! linter [(-> filename File. .getCanonicalPath)]))
"test-resources/valid_syntax.clj"
[]

Expand All @@ -32,14 +34,14 @@
:line pos-int?
:column pos-int?
:warning-details-url "https://github.com/jonase/eastwood#reflection"
:filename "test-resources/eastwood_warning.clj"}
:filename (-> "test-resources/eastwood_warning.clj" File. .getCanonicalPath)}
{:source :eastwood/def-in-def
:line pos-int?
:column pos-int?
:warning-details-url "https://github.com/jonase/eastwood#def-in-def"
:filename "test-resources/eastwood_warning.clj"}
:filename (-> "test-resources/eastwood_warning.clj" File. .getCanonicalPath)}
{:source :eastwood/wrong-pre-post
:line pos-int?
:column pos-int?
:warning-details-url "https://github.com/jonase/eastwood#wrong-pre-post"
:filename "test-resources/eastwood_warning.clj"}]))))
:filename (-> "test-resources/eastwood_warning.clj" File. .getCanonicalPath)}]))))
28 changes: 14 additions & 14 deletions test/integration/formatting_stack/strategies/impl.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,20 @@
(are [dirname filename expected] (= expected
(sut/dir-contains? dirname filename))

"." (-> "src/formatting_stack/strategies/impl.clj" File.) true
(-> "." File. .getAbsolutePath) (File. "project.clj") true
"." (File. "project.clj") true
"." (File. "dev/user.clj") true
"dev" (File. "dev/user.clj") true
"." (File. "LICENSE") true
(-> "." File. .getAbsolutePath) (File. "LICENSE") true
"." (File. "./LICENSE") true
"dev" (File. "LICENSE") false
"dev" (File. "./LICENSE") false
(-> "." File. .getAbsolutePath) (File. "I_dont_exist") false
"." (File. "I_dont_exist") false
"dev" (File. "I_dont_exist") false
"dev" (File. "user.clj") false))
"." (-> "src/formatting_stack/strategies/impl.clj" File.) true
(-> "." File. .getCanonicalPath) (File. "project.clj") true
"." (File. "project.clj") true
"." (File. "dev/user.clj") true
"dev" (File. "dev/user.clj") true
"." (File. "LICENSE") true
(-> "." File. .getCanonicalPath) (File. "LICENSE") true
"." (File. "./LICENSE") true
"dev" (File. "LICENSE") false
"dev" (File. "./LICENSE") false
(-> "." File. .getCanonicalPath) (File. "I_dont_exist") false
"." (File. "I_dont_exist") false
"dev" (File. "I_dont_exist") false
"dev" (File. "user.clj") false))

(deftest absolutize
(are [target] (testing target
Expand Down
2 changes: 1 addition & 1 deletion test/unit/formatting_stack/formatters/cljfmt/impl/magic_symbols.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@
false

(-> (io/file "test-resources" "magic.clj")
(.getAbsolutePath))
(.getCanonicalPath))
true))