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

patch: support linting parent directories #425

Closed
wants to merge 4 commits into from

Conversation

masumsoft
Copy link

I have a setup where we have backend and frontend directories. The backend contains a python-django-rest-api project and frontend is javascript-react-redux project. We wanted to use lint-staged for checking python files in the backend directory too which is under the parent directory.

So we need to remove the pathIsInside check to make it work for both frontend and backend directories.

@codecov
Copy link

codecov bot commented Apr 10, 2018

Codecov Report

Merging #425 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
- Coverage   98.18%   98.15%   -0.03%     
==========================================
  Files          11       11              
  Lines         220      217       -3     
  Branches       25       25              
==========================================
- Hits          216      213       -3     
  Misses          4        4
Impacted Files Coverage Δ
src/generateTasks.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29fc479...01b38aa. Read the comment docs.

@okonet
Copy link
Collaborator

okonet commented Apr 10, 2018

@sudo-suhas can you remember why we added path-inside?

@okonet
Copy link
Collaborator

okonet commented Apr 10, 2018

@masumsoft I'm wondering: did you use lint-staged to work on this PR ;)

@okonet okonet requested a review from sudo-suhas April 10, 2018 12:30
@okonet
Copy link
Collaborator

okonet commented Apr 10, 2018

I'm okay with this change and it's looking good to me. Let's wait for @sudo-suhas to confirm we don't need this check.

@masumsoft
Copy link
Author

@okonet the changes were so simple, that I just used the github editor to update and commit the files and there was no lint staged complaints there you know :P

@sudo-suhas
Copy link
Collaborator

To begin with, this would be a breaking change and I think that in most cases, users would want lint-staged to run the linters on files inside the project only. Open ended file patterns like *.js would match files present anywhere in the git repo, not just the project it was configured in. If you wish to use lint-staged for multiple projects inside your root project, it makes more sense to install it (and husky) in the root of the project. @masumsoft Is this a viable option for you?

The other alternative is to introduce a config flag to optionally disable the path-inside check. I personally prefer we don't go down this route as this is a rare use-case AFAIK.

// Make the paths relative to CWD for filtering
.map(file => path.relative(cwd, file)),
// Make the paths relative to gitDir for filtering
.map(file => path.relative(gitDir, file)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will break existing user config. User would have configured the file patterns relative to current project root as we recommend in the readme.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@okonet We should add a test for this as well.

Copy link
Author

Choose a reason for hiding this comment

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

I can now see it can break existing configs, but it would be great if we could allow relative parents something like ../**.js type paths.

@okonet
Copy link
Collaborator

okonet commented Apr 10, 2018

Makes sense @sudo-suhas. I agree with that, installing and using from root makes more sense to me. Another question: can it be configured with relative paths like ../**.js?

@sudo-suhas
Copy link
Collaborator

can it be configured with relative paths like ../**.js?

No, I do not think so. I'd caution against a project making assumptions about how it is organised outside it's own scope. Earlier with lint-staged@4.x, when we had to configure the file patterns relative to the git root(instead of package.json location), I had voiced a similar concern.

When I am defining the lint-staged config inside a package, I shouldn't have to think about relative paths from a parent folder.

I think this holds for the ../**.js scenario as well.

@sudo-suhas
Copy link
Collaborator

@gangadharjannu
Copy link

How can I run the same front end linter on files outside of the frontend directory.

Example use case backend folder contains few js files and I want to run the same linter for both the backend and front end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants