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

Add modular lint command #109

Closed
NMinhNguyen opened this issue Sep 23, 2020 · 4 comments · Fixed by #643
Closed

Add modular lint command #109

NMinhNguyen opened this issue Sep 23, 2020 · 4 comments · Fixed by #643
Assignees
@cangarugula
Labels
enhancement New feature or request medium priority

Comments

@NMinhNguyen
Copy link
Contributor

See prior discussion in #90 and #108. Will require configurability to specify options such as the base branch (main / master) https://github.com/atlassian/changesets/blob/377f5c385ad9db4ff8458f159e2d452c39828567/packages/config/schema.json#L52-L56
@sebinsua
Copy link
Contributor

sebinsua commented Sep 23, 2020 •
edited
Loading

I wrote two relevant comments within the previous PR:

(1)

Btw, as mentioned in chat, in terms of improving lint within an actual modular project (which this PR is not about), I'm starting to think that it'll be good to add a single modular lint [--all] command which does not support every single eslint option. Because if people really need to use these, they will still be able to skip us and call eslint themselves, and the whole point is to be opinionated.

(2)

I think my position on this is that we kind of aim to fulfill the 'pierceable abstraction' concept by implementing it in such a way that (1) all options are easily passable internally, but (2) the CLI API layer that wraps this is highly opinionated (for example, it might only support --all and --mergeBase=${branchName}).

@NMinhNguyen
Copy link
Contributor Author

I just realised something. Using a naive approach of just calling the CLI (without chunking files like lint-staged does) can cause issues such as lint-staged/lint-staged#147 on Windows. Something to bear in mind for modular lint.

@sebinsua sebinsua added enhancement New feature or request medium priority labels Oct 5, 2020
@sebinsua
Copy link
Contributor

sebinsua commented Oct 6, 2020 •
edited
Loading

Within another issue @NMinhNguyen wrote:

It is also possible to define custom Jest runners: http://youtube.com/watch?v=U_IYuAXtJZ0

I actually quite like this idea with regards to improving the DX of other tools (e.g. modular lint).

@LukeSheard LukeSheard added this to the [email protected] milestone Jul 9, 2021
@LukeSheard
Copy link
Contributor

@cangarugula Let's target this for 1.2 using jest-runner-eslint by default.

@cangarugula cangarugula linked a pull request Jul 21, 2021 that will close this issue
Close #109 Modular lint #643
Merged
LukeSheard added a commit that referenced this issue Jul 23, 2021
@cangarugula @LukeSheard
Close #109 Modular lint (#643)
89574bc 
* lint changed files

* abstract jest-runner-eslint + add onlyDiff option

* added tests & fixture files + default is onlyDiff

* add changeset + fix js fixture

* updated index.test and app.test tests

* update jest-runner-eslint folder name in pkg json

* use git config default branch if no remote origin

* use ternary

* update yarn.lock

* update getworkspaceinfo snapshot

* clean up snapshot

* moved jest.setTimeout + eslint-config-modular-app

* refresh yarn

* use modular lint in lint-staged

* delete config.js file and move into index.js

* update snapshot

* update snapshots

Co-authored-by: Cang Truong <[email protected]>
Co-authored-by: Luke Sheard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cangarugula cangarugula

Labels
enhancement New feature or request medium priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Close #109 Modular lint jpmorganchase/modular
4 participants
@sebinsua @NMinhNguyen @LukeSheard @cangarugula