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 developer overview #385

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Mar 29, 2022

.. and fix problem with pre-commit! Excludes doesn't work as we thought.

At least for black, black's own config isn't used at all. It is pre-commit that selects which of the already indexed files to check and that is controlled with the "exclude", "language" and "types" arguments. "exclude" is a regular expression.

See https://stackoverflow.com/questions/61032281/exclude-some-files-on-running-black-using-pre-commit

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.20%. Comparing base (d114b38) to head (e24ca28).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #385   +/-   ##
=======================================
  Coverage   84.20%   84.20%           
=======================================
  Files          89       89           
  Lines        4040     4040           
=======================================
  Hits         3402     3402           
  Misses        638      638           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lunkwill42
Copy link
Member

.. and fix problem with pre-commit! Excludes doesn't work as we thought.

At least for black, black's own config isn't used at all. It is pre-commit that selects which of the already indexed files to check and that is controlled with the "exclude", "language" and "types" arguments. "exclude" is a regular expression.

See https://stackoverflow.com/questions/61032281/exclude-some-files-on-running-black-using-pre-commit

Actually, as explained in the link you posted, it's not that Black's config isn't being used: Black still reads its config from pyproject.toml in our case. However, Blacks exclude-option is only used to exclude files when Black is discovering the file tree itself. If a filename is explicitly put as an argument to the black command line (which seems to be what the pre-commit framework is doing), Black's exclude option doesn't apply at all.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

This is a good birds-eye view - but it does comes dangerously close to being just a glossary of terms. Could Sphinx' glossary functionality be leveraged in some way here, perhaps?

https://sublime-and-sphinx-guide.readthedocs.io/en/latest/glossary.html

docs/development/birdseye-view.rst Outdated Show resolved Hide resolved
docs/development/birdseye-view.rst Outdated Show resolved Hide resolved
@hmpf
Copy link
Contributor Author

hmpf commented Apr 25, 2022

This is a good birds-eye view - but it does comes dangerously close to being just a glossary of terms. Could Sphinx' glossary functionality be leveraged in some way here, perhaps?

I'll play around a bit. Not sure if one can have more than one glossary, if ever we need another figure like this in the same docs.

@hmpf
Copy link
Contributor Author

hmpf commented Apr 25, 2022

Hm, Sphinx glossary is not hierarchical, so there would be no good way to both discuss sources and types of sources without a major rewrite.

or otherwise handled by the glue-service.

The glue-service can either be built in to the source, or be completely
standalone and pull changes from the source before it pushes them on.
Copy link
Contributor

Choose a reason for hiding this comment

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

"pushes them to Argus" instead of "pushes them on", maybe?


A notification profile is used by the server for sending notifications about
a chosen subset of the incidents. It consists of one or more filters, one or
more timeslots (for which periods of the day a profile is valid) and one or
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually just one timeslot - even after we make timeslots reusable

@hmpf hmpf force-pushed the developer-overview branch from f817669 to 4bff91b Compare May 3, 2022 11:31
Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

Looks good, but suggestions from @johannaengland have merit and should be included.

@hmpf hmpf added the rc-future label Oct 10, 2022
@lunkwill42 lunkwill42 removed the request for review from stveit October 14, 2022 06:23
@hmpf hmpf added the paused Assignee is busy with things of higher priority label Nov 15, 2022
@hmpf hmpf self-assigned this Nov 16, 2022
@hmpf hmpf force-pushed the developer-overview branch from 4bff91b to 6469b1a Compare April 25, 2024 13:06
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

Test results

       5 files         1 errors  328 suites   11m 41s ⏱️
   462 tests    461 ✔️ 1 💤 0
1 848 runs  1 844 ✔️ 4 💤 0

For more details on these parsing errors, see this check.

Results for commit 6469b1a.

@hmpf hmpf force-pushed the developer-overview branch from 6469b1a to 00a5073 Compare September 13, 2024 10:49
hmpf and others added 4 commits October 4, 2024 10:07
Turns out we had misunderstood how pre-commit excludes files. When
running at least "black" from pre-commit, black doesn't use its own
config files at all, including any list of excluded files.
Co-authored-by: Morten Brekkevold <100995+lunkwill42@users.noreply.github.com>
Co-authored-by: Morten Brekkevold <100995+lunkwill42@users.noreply.github.com>
@hmpf hmpf force-pushed the developer-overview branch from 00a5073 to e24ca28 Compare October 4, 2024 08:09
Copy link

sonarqubecloud bot commented Oct 4, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paused Assignee is busy with things of higher priority rc-future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants