Gracefully handle Git deletions again #137

vemv · 2020-02-27T08:55:06Z

Brief

impl/absolutize, particularly with its .exists assertion, had broken our ability to gracefully handle deleted files (staged or not).

This PR fixes that, adding an extensive integration test.

QA plan

Test all cases in a real project (suggestion: a monorepo):

Deleted file (staged for deletion or not)
plain core/format!, format-and-lint-project!, format-and-lint-branch!

That's 6 scenarios - please let's test them all (I have in formatting-stack itself).

Author checklist

Reviewer checklist

… cases

thumbnail

Good work 👍 extensive tests as well :)

src/formatting_stack/strategies.clj

thumbnail · 2020-02-28T07:36:16Z

test/integration/formatting_stack/strategies.clj

+
+(assert (-> deletable-file .exists))
+
+(assert (not (-> createable-filename File. .exists)))
+


These should be inside of the test. The files don't have to exist/not-exists when loading this ns, they should when i run the tests

These should be inside of the test.

Note that 2 deftests depend on this assertion. Having 2 deftests fail seems less clear, concise and immediate than having (refresh) fail as soon as an issue arises.

Except that now i can refresh, delete the file and run the test. yielding unclear failures as well.

Except that now i can refresh, delete the file and run the test.

Running tests without a prior (refresh) seems just wrong. Our dev.clj file typically suggests to (refresh) prior to running any test

test/integration/formatting_stack/strategies.clj

vemv · 2020-02-28T10:02:34Z

Ready again!

thumbnail

during QA i noticed that absolutize might not work as expected in monorepos:

The module part is missing from the path. (e.g. project-dir/src is used instead of project-dir/module/src)

vemv · 2020-03-02T09:36:42Z

👀!

Did you get exceptions? Supposedly there's a spec that would assert impl/absolutize output would correspond to an existing file

thumbnail · 2020-03-02T09:46:49Z

Supposedly there's a spec that would assert impl/absolutize output would correspond to an existing file

It's probably happening when *skip-existing-files-check?* is bound to true

vemv · 2020-03-02T11:33:43Z

Did you get exceptions?

(For clarity: at least the spec checking part was working, w/ nice Expound reports)

thumbnail · 2020-03-02T11:39:21Z

Did you get exceptions?

Only the ExceptionInfo-exceptions with the validation errors.

-- Relevant specs -------

:formatting-stack.strategies.impl/existing-files:
  (clojure.spec.alpha/coll-of
   (nedap.speced.def/fn
    [s]
    (if
     formatting-stack.strategies.impl/*skip-existing-files-check?*
     true
     (clojure.core/-> s File. .exists))))

-- Spec failed --------------------

  (...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   "<SNIP>"
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...
   ...)

should satisfy

  (speced/fn
   [s]
   (if *skip-existing-files-check?* true (-> s File. .exists)))

-- Relevant specs -------

:formatting-stack.strategies.impl/existing-files:
  (clojure.spec.alpha/coll-of
   (nedap.speced.def/fn
    [s]
    (if
     formatting-stack.strategies.impl/*skip-existing-files-check?*
     true
     (clojure.core/-> s File. .exists))))

-------------------------
Detected 27 errors

Execution error (ExceptionInfo) at formatting-stack.strategies/all-files$fn (strategies.clj:47).
Validation failed

vemv · 2020-03-02T11:46:04Z

Some notes:

git status porcelain always emits paths relative to the git root, not to the cwd
git diff --name-status always emits paths relative to the git root, not to the cwd
git ls-files, at least by default emits paths relative to the cwd

The latter, beng different, is a source of issues.

This makes `ls-files` option more analog to that of `diff --name-status` and `status --porcelain`, in that it is independent from the current directory (which is relevant for monorepos)

vemv · 2020-03-02T12:02:49Z

Kindly QA again. I'll be also, over a non-monorepo

thumbnail · 2020-03-02T12:39:12Z

staged/unstaged deleted files within the same module work as expected. However strategies/all-files only returns the files present in (System/getProperty "user.dir") (e.g. the module itself). Not the files outside of the module.

vemv · 2020-03-02T12:44:00Z

I thought about that.

It seems bit of a poor default to return files outside of the monorepo. Most folks will not want files outside of the current project to be returned.

Else they will be formatting/linting files outside of what Clojure can require, so bugs would arise.

That's a non-issue for us with the :monorepo profile, or people doing similar stuff (https://clubhouse.io/blog/monolith-meet-mono-repo/). But it seems unsafe to assume that everyone will have that set up.

(Even among us :monorepo is optional)

thumbnail · 2020-03-02T12:47:01Z

Else they will be formatting/linting files outside of what Clojure can require, so bugs would arise.

This is not necessarily true, the files might be on the classpath, but not in the current project. It might be specific to our monorepo setup, but running project-formatter/format-and-lint-project! for each project could make sense 🤔

vemv · 2020-03-02T12:53:19Z

This is not necessarily true, the files might be on the classpath, but not in the current project.

Yes, but we cannot know with all certainty.

(Monorepo != "all projects can reach each other's set of namespaces")

Created #139 + #140 btw, which can be attended promptly but would not consider a blocker (as f-s is somewhat broken rn)

vemv added 8 commits February 27, 2020 09:54

Gracefully handle Git deletions again

93ee8db

Add integration.formatting-stack.strategies

3a32661

Adapt a test

fe1fa64

Add deletable.clj test resource

6f6567e

Complete integration.formatting-stack.strategies

2458453

Expand integration.formatting-stack.strategies

c0f56ac

Remove some accidental duplication

b924dc7

Prevent some gotchas in integration.formatting-stack.strategies

e9586a2

vemv added the squash-merge label Feb 28, 2020

vemv added 3 commits February 28, 2020 01:21

Extract strategies.impl.git-status ns

5a31922

integration.formatting-stack.strategies: exercise deletions in more…

d57a20b

… cases

Typos

b0cbb64

vemv changed the base branch from master to cljs-old--v2 February 28, 2020 03:41

Add a comment

7ccfd8c

vemv marked this pull request as ready for review February 28, 2020 03:54

vemv requested a review from thumbnail February 28, 2020 03:54

vemv changed the title ~~[WIP] Gracefully handle Git deletions again~~ Gracefully handle Git deletions again Feb 28, 2020

thumbnail requested changes Feb 28, 2020

View reviewed changes

vemv changed the base branch from cljs-old--v2 to master February 28, 2020 09:31

vemv added 2 commits February 28, 2020 10:31

Merge branch 'master' into deleted

d102b7f

PR feedback

8bc1d07

vemv added 2 commits February 28, 2020 11:05

rm satisfied reminder

feba4eb

Add an assertion

2e385f2

thumbnail approved these changes Feb 28, 2020

View reviewed changes

thumbnail requested changes Mar 2, 2020

View reviewed changes

git ls-files: use --full-name option

c2b3c4a

This makes `ls-files` option more analog to that of `diff --name-status` and `status --porcelain`, in that it is independent from the current directory (which is relevant for monorepos)

vemv assigned thumbnail Mar 2, 2020

vemv mentioned this pull request Mar 2, 2020

Reify policy for monorepos #140

Open

thumbnail approved these changes Mar 2, 2020

View reviewed changes

vemv merged commit c1455b6 into master Mar 2, 2020

vemv mentioned this pull request Mar 2, 2020

v4.0.1 #141

Merged

7 tasks

thumbnail deleted the deleted branch March 2, 2020 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gracefully handle Git deletions again #137

Gracefully handle Git deletions again #137

vemv commented Feb 27, 2020 •

edited

Loading

thumbnail left a comment

thumbnail Feb 28, 2020

vemv Feb 28, 2020

thumbnail Feb 28, 2020

vemv Feb 28, 2020

vemv commented Feb 28, 2020

thumbnail left a comment

vemv commented Mar 2, 2020

thumbnail commented Mar 2, 2020

vemv commented Mar 2, 2020

thumbnail commented Mar 2, 2020

vemv commented Mar 2, 2020

vemv commented Mar 2, 2020

thumbnail commented Mar 2, 2020

vemv commented Mar 2, 2020 •

edited

Loading

thumbnail commented Mar 2, 2020

vemv commented Mar 2, 2020


		(assert (-> deletable-file .exists))

		(assert (not (-> createable-filename File. .exists)))

Gracefully handle Git deletions again #137

Gracefully handle Git deletions again #137

Conversation

vemv commented Feb 27, 2020 • edited Loading

Brief

QA plan

Author checklist

Reviewer checklist

thumbnail left a comment

Choose a reason for hiding this comment

thumbnail Feb 28, 2020

Choose a reason for hiding this comment

vemv Feb 28, 2020

Choose a reason for hiding this comment

thumbnail Feb 28, 2020

Choose a reason for hiding this comment

vemv Feb 28, 2020

Choose a reason for hiding this comment

vemv commented Feb 28, 2020

thumbnail left a comment

Choose a reason for hiding this comment

vemv commented Mar 2, 2020

thumbnail commented Mar 2, 2020

vemv commented Mar 2, 2020

thumbnail commented Mar 2, 2020

vemv commented Mar 2, 2020

vemv commented Mar 2, 2020

thumbnail commented Mar 2, 2020

vemv commented Mar 2, 2020 • edited Loading

thumbnail commented Mar 2, 2020

vemv commented Mar 2, 2020

vemv commented Feb 27, 2020 •

edited

Loading

vemv commented Mar 2, 2020 •

edited

Loading