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

Delete old .pre-commit-config.yaml files #176

Merged
merged 12 commits into from
Feb 20, 2023
Merged

Conversation

dekkers
Copy link
Contributor

@dekkers dekkers commented Feb 15, 2023

No description provided.

@dekkers dekkers requested a review from a team as a code owner February 15, 2023 13:39
@github-actions
Copy link
Contributor

github-actions bot commented Feb 15, 2023

File Coverage
All files 67%
bits/definitions.py 64%
bits/runner.py 56%
bits/https_availability/https_availability.py 93%
bits/oois_in_headers/oois_in_headers.py 72%
bits/spf_discovery/internetnl_spf_parser.py 55%
bits/spf_discovery/spf_discovery.py 77%
octopoes/api/api.py 89%
octopoes/api/models.py 75%
octopoes/api/router.py 56%
octopoes/core/app.py 69%
octopoes/core/service.py 58%
octopoes/events/events.py 96%
octopoes/events/manager.py 65%
octopoes/models/__init__.py 80%
octopoes/models/datetime.py 66%
octopoes/models/exception.py 83%
octopoes/models/origin.py 70%
octopoes/models/path.py 99%
octopoes/models/types.py 95%
octopoes/models/ooi/certificate.py 95%
octopoes/models/ooi/email_security.py 95%
octopoes/models/ooi/findings.py 94%
octopoes/models/ooi/network.py 97%
octopoes/models/ooi/service.py 91%
octopoes/models/ooi/software.py 71%
octopoes/models/ooi/web.py 81%
octopoes/models/ooi/dns/records.py 95%
octopoes/models/ooi/dns/zone.py 82%
octopoes/repositories/ooi_repository.py 40%
octopoes/repositories/origin_parameter_repository.py 52%
octopoes/repositories/origin_repository.py 52%
octopoes/repositories/scan_profile_repository.py 45%
octopoes/xtdb/client.py 39%
octopoes/xtdb/query_builder.py 69%
octopoes/xtdb/related_field_generator.py 73%
tests/conftest.py 91%

Minimum allowed coverage is 75%

Generated by 🐒 cobertura-action against 109f9c7

@Donnype
Copy link
Contributor

Donnype commented Feb 15, 2023

@dekkers Could you perhaps also set exclude = ["/tests/", ".venv"] in the main pyproject.toml? And since settings from the pyproject.toml seem to be ignored in the sub-services, can't we remove most of these (or at least parts for keiko and octopoes) as well?

@ammar92
Copy link
Contributor

ammar92 commented Feb 16, 2023

This is fine for me (for now), although I still see benefits to using specific configurations for (several) services:

  • They are a service/ application on their own and can be pulled and built as such
  • Although we want to keep this as minimal as possible, the root configuration might get cluttered with a lot of exceptions/includes/excludes/hooks/etc. that would otherwise be in the other configuration files
  • I'm not sure if running the (linter) hooks from the main configuration will respect the configuration (mostly in pyproject.toml or .ini files) in the service/ application folders

pyproject.toml Outdated
@@ -20,5 +20,5 @@ max-line-length = 120

[tool.vulture]
min_confidence = 90
exclude = ["/tests/"]
exclude = ["/tests/", "/.venv/"]
Copy link
Contributor

@ammar92 ammar92 Feb 16, 2023

Choose a reason for hiding this comment

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

Hmmm not sure about this change:

  • .venv seems to be your local installation-specific folder; as many others use just venv for example (I haven't seen many usages of .venv tbh)
  • Not sure if this is even needed, because pre-commit only run on staged files (and venv folders are definitely igorned)
  • If we really must include this in the configuration then I would opt for the option "*venv*" instead

Copy link
Contributor

Choose a reason for hiding this comment

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

This was actually an issue I was experiencing when running linters, and vulture was taking forever parsing my .venv's

Copy link
Contributor

Choose a reason for hiding this comment

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

That depends entirely on how you run it.

I've just tested this with pre-commit without the exclude option and it worked fine. Also doesn't seem to be an issue for the CI

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline: vulture does not deal elegantly with the files it receives from pre-commit on all platforms for some reason. For the time being, let's add "*venv*" as a "temporary" solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The vulture hook uses pass_filenames: false so doesn't get the list of files from pre-commit. The other hooks don't use that option so don't have this problem. "*venv*" might accidentally match too much, I've added both "/venv/" and "/.venv/" now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good catch. I'm fine with this. Although I would prefer to have pass_filenames enabled and add the venv patterns to the root exclude. This would make the rest of the config files a bit cleaner since we don't have to repeatedly add these patterns.

Donnype
Donnype previously approved these changes Feb 16, 2023
ammar92
ammar92 previously approved these changes Feb 18, 2023
@Darwinkel Darwinkel merged commit 258c781 into main Feb 20, 2023
@Darwinkel Darwinkel deleted the remove-old-precommit-configs branch February 20, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants