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

feat: ignore warnings via .nsci-ignore file #7

Merged
merged 26 commits into from
Jul 3, 2022
Merged

Conversation

tony-go
Copy link
Member

@tony-go tony-go commented Jun 9, 2022

Resolve issue #4 (Milestone 1).

You'll be able to ignore warnings with a .nsci-ignore file:

{
  "warnings": {
    "unsafe-stmt": ["lodash.difference", "regenerator-runtime"]
  }
}

@tony-go tony-go added the enhancement New feature or request label Jun 9, 2022
@tony-go tony-go self-assigned this Jun 9, 2022
@tony-go tony-go marked this pull request as ready for review June 20, 2022 15:41
@tony-go
Copy link
Member Author

tony-go commented Jun 20, 2022

Definitely not my definitive version, but I'd like to have the first review.

@tony-go tony-go requested a review from antoine-coulon June 20, 2022 15:42
Copy link
Member

@antoine-coulon antoine-coulon left a comment

Choose a reason for hiding this comment

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

Good job on that PR!
I know that its still a wip so feel free to suggest things about my feedback or even other things

src/analysis/interpretation/interpret.ts Outdated Show resolved Hide resolved
src/configuration/external/adapt.ts Outdated Show resolved Hide resolved
src/configuration/external/adapt.ts Outdated Show resolved Hide resolved
src/configuration/external/nodesecure/ignore-file.ts Outdated Show resolved Hide resolved
src/configuration/external/nodesecure/index.ts Outdated Show resolved Hide resolved
src/configuration/external/nodesecure/index.ts Outdated Show resolved Hide resolved
src/configuration/external/nodesecure/index.ts Outdated Show resolved Hide resolved
src/configuration/standard/nsci.ts Outdated Show resolved Hide resolved
src/configuration/external/standardize.ts Show resolved Hide resolved
src/analysis/interpretation/interpret.ts Outdated Show resolved Hide resolved
@tony-go tony-go force-pushed the feature/nsci-ignore branch from 0b1fca4 to 02e0458 Compare June 23, 2022 11:12
@tony-go
Copy link
Member Author

tony-go commented Jun 23, 2022

Made corrections cc @antoine-coulon

Let's add e2e tests now, and enrich existing ones.

Then we should be good for milestone 1.

@antoine-coulon
Copy link
Member

Made corrections cc @antoine-coulon

Let's add e2e tests now, and enrich existing ones.

Then we should be good for milestone 1.

LGTM

@tony-go
Copy link
Member Author

tony-go commented Jun 29, 2022

I'll make a finally review, to see if I didn't have duplicated stuff, magic numbers etc...

Before that I have two remaining question marks:

  • I think we should have a consistent naming convention for RC and ignore file, WDYT
    • .nsci-ignore & .nsci-rc
    • .nodesecureignore & .nodesecurerc
  • @antoine-coulon could you take a look at how I log stuff in getIgnoreFile function and tell me if I'm good or not... I think that use something that should be dedicated to reporting. Let me know. In case I'll create a logger abstract that we could share across the codebase and that won't output in the context of test env.

@antoine-coulon
Copy link
Member

I'll make a finally review, to see if I didn't have duplicated stuff, magic numbers etc...

Before that I have two remaining question marks:

  • I think we should have a consistent naming convention for RC and ignore file, WDYT

    • .nsci-ignore & .nsci-rc
    • .nodesecureignore & .nodesecurerc

You're completely right, I would go for the latest one, .nodesecureignore & .nodesecurerc.

  • @antoine-coulon could you take a look at how I log stuff in getIgnoreFile function and tell me if I'm good or not... I think that use something that should be dedicated to reporting. Let me know. In case I'll create a logger abstract that we could share across the codebase and that won't output in the context of test env.

In my humble opinion it's fine to directly use reporting utils instead of creating a dedicated reporter abstracting that, as it appears that the logged stuff is really light.

However for the output part in the context of tests you're right, it should not appear. One way of fixing without two much overhead would be using some sort of dependency injection directly in the getIgnoreFile function. Consequently with DI you could do a test implementation that prints nothing for each of the reporter's methods.

Ideally, this should be done at an higher application level to control and inject all other reporters the same way but it's clearly out of the scope of the PR.

Copy link
Member

@fraxken fraxken left a comment

Choose a reason for hiding this comment

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

LGTM

@tony-go
Copy link
Member Author

tony-go commented Jul 2, 2022

Finally ready for the big jump @antoine-coulon

Copy link
Member

@antoine-coulon antoine-coulon left a comment

Choose a reason for hiding this comment

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

LGTM

@tony-go tony-go merged commit 407c818 into main Jul 3, 2022
@tony-go tony-go deleted the feature/nsci-ignore branch July 3, 2022 13:55
tony-go added a commit that referenced this pull request Jul 3, 2022
parent 4f7b4ef
author Tony Gorez <[email protected]> 1654807466 +0200
committer Tony Gorez <[email protected]> 1656869571 +0200

feat: add getIngoreFile fn

chore: delete di and use mock-fs

feat: make validator return error

fix: root path

feat: add base filter

fix: types

chore(interpret): create hasWarningsIgnorePatterns func

chore(adapt): fix import)

fix(adapt): dont return ignorePatterns

fix(ignore-file): add jsxray types

fix: use process.cwd()

fix: use IgnorePatterns.default instead of creating manual object

fix: lint + use IgnorePatterns.default

fix: standardizeExternalConfiguration type

chore: rename filter function

chore: rename a few variables/func/types

test: move test and fix types

chore: apply linter

chore(interpret): add fixture generators

chore: rename ignore file

fix: create temporary logger abstract

chore: apply linter

doc: add .nodesecureignore base doc

fix: IgnorePatterns & IgnoreWarningsPatterns abstract
@antoine-coulon
Copy link
Member

@all-contributors please add @tony-go for code, doc

@allcontributors
Copy link
Contributor

@antoine-coulon

I've put up a pull request to add @tony-go! 🎉

antoine-coulon pushed a commit that referenced this pull request Aug 30, 2022
parent 4f7b4ef
author Tony Gorez <[email protected]> 1654807466 +0200
committer Tony Gorez <[email protected]> 1656869571 +0200

feat: add getIngoreFile fn

chore: delete di and use mock-fs

feat: make validator return error

fix: root path

feat: add base filter

fix: types

chore(interpret): create hasWarningsIgnorePatterns func

chore(adapt): fix import)

fix(adapt): dont return ignorePatterns

fix(ignore-file): add jsxray types

fix: use process.cwd()

fix: use IgnorePatterns.default instead of creating manual object

fix: lint + use IgnorePatterns.default

fix: standardizeExternalConfiguration type

chore: rename filter function

chore: rename a few variables/func/types

test: move test and fix types

chore: apply linter

chore(interpret): add fixture generators

chore: rename ignore file

fix: create temporary logger abstract

chore: apply linter

doc: add .nodesecureignore base doc

fix: IgnorePatterns & IgnoreWarningsPatterns abstract
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants