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

chore: docs and config maintenance #105

Merged

Conversation

jpower432
Copy link
Contributor

@jpower432 jpower432 commented Dec 8, 2023

Description

Maintenance updates on docs and configuration files:

  • Updates semgrep version in pre-commit and add make lint to pre-commit file
  • Update Contributing guide with developer tool information
  • Update dev container to install pre-commit on create
  • Update megalinter config

Really focusing on the CONTRIUBTING.md changes here. The point of the changes is to expand on the tools used for development and answer the 5 W's for each of them.

Type of change

  • This change requires a documentation update

How has this been tested?

A codespace has been created from this branch. I verified that the pre-commit hooks were installed and it included the linting check before commiting.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Bumps semgrep version to 1.51.0 and adds a local check for `make lint`

Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
To ensure warnings/violations are not missed, remove the
linter with disable errors and move to enabled.

Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
…ools

Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
@jpower432 jpower432 added the documentation Improvements or additions to documentation label Dec 8, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

Wanted to understand why you added the && make pre-commit for my own benefit, we can discuss during our next daily

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. This was added to make sure that the pre-commit hooks are installed in the dev container environment. They can be uninstalled later if the dev using the environment wants to do that, but it makes sense to me to install them by default to reduce errors and additional steps for the environment configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wanted to understand the hooks added here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linting checks are already run in CI for verification, but it is faster to have them run as a pre-commit check. Currently make lint is just run manually, but adding will reduce the chance that the pipeline will fail only because of a linting error. This is running the same command as the check in CI to ensure that the version of the linters are the same and run with the same configuration.

Copy link
Contributor

@beatrizmcouto beatrizmcouto left a comment

Choose a reason for hiding this comment

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

LGTM. Left a couple of comments for things which I would like to better understand for our next daily

Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
@jpower432
Copy link
Contributor Author

LGTM. Left a couple of comments for things which I would like to better understand for our next daily

Thanks @beatrizmcouto! Added a table of contents to the guide as discussed and I will answer the other questions.

Copy link
Contributor

@beatrizmcouto beatrizmcouto left a comment

Choose a reason for hiding this comment

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

LGTM

@jpower432 jpower432 merged commit ee5cf32 into complytime:main Jan 3, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants