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.

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

Jump to bottom

Eastwood: always use absolute filenames, internally #187

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

Eastwood: always use absolute filenames, internally #187

wants to merge 1 commit into from

Conversation

vemv
Copy link
Contributor

@vemv vemv commented Mar 30, 2021

Brief

Part of #169

As a prerequisite for #169, I found out that filenames coming from Eastwood reports should be absolute, otherwise they wouldn't match with filenames computed by a 'linter caching' mechanism. Of course, a key mismatch means no cache hits.

This is the data path as of master:

  • f-s sends an absolutized filename (via our git strategies) to Eastwood
  • Eastwood may return wither a File (which will be stringed, absolutized) or a string (which will be a relative filename)

This PR proposes making the data path always absolutized filename -> absolutized filename.

QA plan

Verify that Eastwood keeps emitting reports when a fault arises, in vanilla repos and monorepos alike.

⚠️The report should be rendered with relativized filenames (as it's a friendly/concise UI), as always.

Author checklist

  • I have QAed the functionality
  • The PR has a reasonably reviewable size and a meaningful commit history
  • I have run the branch formatter and observed no new/significative warnings
  • The build passes
  • I have self-reviewed the PR prior to assignment
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

Reviewer checklist

  • I have checked out this branch and reviewed it locally, running it
  • I have QAed the functionality
  • I have reviewed the PR
  • Additionally, I have code-reviewed iteratively the PR considering the following aspects in isolation:
    • Correctness
    • Robustness (red paths, failure handling etc)
    • Test coverage
    • Spec coverage
    • Documentation
    • Security
    • Performance
    • Breaking API changes
    • Cross-compatibility (Clojure/ClojureScript)

@vemv vemv force-pushed the 169--eastwood-absolutize branch 4 times, most recently from e7d449f to 083236c Compare March 30, 2021 01:52
vemv
vemv commented Mar 30, 2021
View reviewed changes
Copy link
Contributor Author

@vemv vemv left a comment

Choose a reason for hiding this comment

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

(Ready-ish but I should QA it)

e2e/monorepo-support/test/monorepo_support/core_test.clj
@@ -22,7 +22,7 @@

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

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

src/formatting_stack/linters/eastwood.clj
@@ -28,6 +32,14 @@
(note that this prefix must not be passed to Eastwood itself).")

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

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

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

@vemv
Copy link
Contributor Author

vemv commented Sep 3, 2021

(This branch has worked well for me in my personal setup. Will undust this PR soon enough)

@vemv vemv force-pushed the 169--eastwood-absolutize branch from 083236c to 9b753a4 Compare September 23, 2021 01:32
@vemv vemv force-pushed the 169--eastwood-absolutize branch from 9b753a4 to a120efd Compare September 23, 2021 01:33
@vemv
Copy link
Contributor Author

vemv commented Sep 23, 2021

Rebased, gonna give it some extra QAing after this updating

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

@thumbnail thumbnail thumbnail left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vemv @thumbnail