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

add documentation on ignores #46

Merged
merged 4 commits into from
Nov 13, 2020
Merged

add documentation on ignores #46

merged 4 commits into from
Nov 13, 2020

Conversation

ievans
Copy link
Member

@ievans ievans commented Nov 12, 2020

@dlukeomalley -- after this lands we can move a lot of the semgrep action readme to just point here

@pabloest -- do you think "Ignoring Findings" should be its own, top-level section? I think that's a good idea

@@ -31,7 +31,9 @@ $ semgrep -e '$X == $X' --lang=py path/to/src
$ semgrep --config path/to/yaml
```

# Managing findings
# Ignoring findings
Copy link
Member

Choose a reason for hiding this comment

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

n.b. - we'll need to add redirects for the docs before merging title changes (because they are used in existing links)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly we cannot use redirects here, because any content after the # is not passed to the server. Thoughts on how to handle? IMO this is an argument for having more pages rather than fewer pages with lots of local anchor # links.

Copy link
Member

Choose a reason for hiding this comment

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

A crude way would just be to add an HTML element to catch that link and place it right next to the updated header. Something like: <div id="managing-findings"></div>

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point @pabloest . I ended up just putting the header back in but linking ignore findings.

docs/running-rules.md Outdated Show resolved Hide resolved
@pabloest
Copy link
Member

@dlukeomalley -- after this lands we can move a lot of the semgrep action readme to just point here

@pabloest -- do you think "Ignoring Findings" should be its own, top-level section? I think that's a good idea

I think we should keep plenty of info in the semgrep action readme because it is auto-shown on the GitHub marketplace page: https://github.com/marketplace/actions/semgrep-action

I agree on the ignoring findings thing.

@ievans ievans merged commit 4f516ca into main Nov 13, 2020
@ievans ievans deleted the ie/ignoring branch November 13, 2020 20:35
@@ -13,7 +13,7 @@ Explore the Semgrep docs and join an amazing community of engineering and securi
- [Running rules](running-rules.md)
- [Managing CI policy](managing-policy.md)
- [Integrations](integrations.md)
- [Experiments](experiments.md)
- [Experiments](experiments)
Copy link
Member

Choose a reason for hiding this comment

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

What led to this change? Did the link not work before? 😬

semgrep-core/tests/
```

For a complete example, see [the .semgrepignore on the Semgrep source code itself](https://github.com/returntocorp/semgrep/blob/develop/.semgrepignore).
Copy link
Member

Choose a reason for hiding this comment

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

nit: use ` around .semgrepignore for consistency with rest of text.

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