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

journald: basic ruff formating #139

Merged
merged 10 commits into from
Aug 3, 2023
Merged

journald: basic ruff formating #139

merged 10 commits into from
Aug 3, 2023

Conversation

konstruktoid
Copy link
Contributor

Ref #135

Signed-off-by: Thomas Sjögren <[email protected]>
Signed-off-by: Thomas Sjögren <[email protected]>
@konstruktoid
Copy link
Contributor Author

Will fix line length later today

Alex-Izquierdo
Alex-Izquierdo previously approved these changes Jun 22, 2023
Copy link
Contributor

@Alex-Izquierdo Alex-Izquierdo left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Thomas Sjögren <[email protected]>
@konstruktoid
Copy link
Contributor Author

All checks passed 👍

@ccamacho
Copy link
Contributor

ccamacho commented Jun 22, 2023

Hi! all the fixes LGTM but there are still a few things here (PRs will always pass because this is not tested), I just fetched/rebased #139 on top of #138 and this is part of the output:


linters: commands[0]> bash -c 'echo '"'"'==> Installing Event Driven Ansible...'"'"''
==> Installing Event Driven Ansible...
linters: commands[1]> bash -c 'ansible-galaxy collection build -v --force --output-path releases/ && ansible-galaxy collection install --force --force-with-deps releases/ansible-eda-$(cat ./galaxy.yml | shyaml get-value version).tar.gz'
No config file found; using defaults
Created collection for ansible.eda at /home/ccamacho/dev/event-driven-ansible/releases/ansible-eda-1.3.8.tar.gz
Starting galaxy collection install process
Process install dependency map
Starting collection install process
Installing 'ansible.eda:1.3.8' to '/home/ccamacho/.ansible/collections/ansible_collections/ansible/eda'
ansible.eda:1.3.8 was installed successfully

linters: commands[2]> bash -c 'ruff check --exclude .tox --select ALL --ignore INP001 . || true'
.
.
.
extensions/eda/plugins/event_source/journald.py:32:11: ANN201 Missing return type annotation for public function `main`
extensions/eda/plugins/event_source/journald.py:32:11: D103 Missing docstring in public function
extensions/eda/plugins/event_source/journald.py:58:11: D101 Missing docstring in public class
extensions/eda/plugins/event_source/journald.py:59:19: ANN201 Missing return type annotation for public function `put`
extensions/eda/plugins/event_source/journald.py:59:19: D102 Missing docstring in public method
extensions/eda/plugins/event_source/journald.py:59:23: ANN101 Missing type annotation for `self` in method
extensions/eda/plugins/event_source/journald.py:59:29: ANN001 Missing type annotation for function argument `event`
extensions/eda/plugins/event_source/journald.py:60:13: T201 `print` found
.
.
.
.
.
.
.
Found 739 errors.

linters: commands[3]> bash -c 'find . -not -path "./.tox/*" -and -name "*.py" -print0 | xargs -0 darglint -v 1 -s numpy -z full || true'
.
.
.
linters: commands[4]> bash -c 'find . -not -path "./.tox/*" -and -name "*.py" -print0 | xargs -0 pylint || true'
.
.
.
************* Module journald
extensions/eda/plugins/event_source/journald.py:32:0: C0116: Missing function or method docstring (missing-function-docstring)
extensions/eda/plugins/event_source/journald.py:58:4: C0115: Missing class docstring (missing-class-docstring)
extensions/eda/plugins/event_source/journald.py:59:8: C0116: Missing function or method docstring (missing-function-docstring)
extensions/eda/plugins/event_source/journald.py:58:4: R0903: Too few public methods (1/2) (too-few-public-methods)
.
.
.

linters: commands[5]> bash -c 'flake8 --exclude .tox || true'
.
./tests/integration/event_source_url_check/test_url_check_source.py:45:1: DAR101 Missing parameter(s) in Docstring: - endpoint
.
  linters: OK (14.89=setup[0.06]+cmd[0.01,1.18,0.18,0.37,12.53,0.56] seconds)
  congratulations :) (14.95 seconds)

There are still a few things to fix before it passes.

I would vote first to have the lint check merged (but disabled like in #138) so it is possible to verify that the styling is actually consistent. Future tests can be easily verified and merged.

@konstruktoid
Copy link
Contributor Author

Will fix during the weekend, and use ruff check --exclude .tox --select ALL --ignore INP001 . instead of the current GitHub tests

@konstruktoid
Copy link
Contributor Author

rebased

Signed-off-by: Thomas Sjögren <[email protected]>
@konstruktoid
Copy link
Contributor Author

Failing tox test: #149

@ttuffin ttuffin requested review from Alex-Izquierdo and a team July 5, 2023 12:34
ttuffin
ttuffin previously approved these changes Jul 5, 2023
@Alex-Izquierdo
Copy link
Contributor

Hi @konstruktoid, it looks that the mentioned PR that is already merged is not enough to pass the ruff checks.

@konstruktoid
Copy link
Contributor Author

Will take a look

@konstruktoid
Copy link
Contributor Author

All checks have passed.

ref astral-sh/ruff#5571

@ttuffin ttuffin merged commit 0d858d9 into ansible:main Aug 3, 2023
ttuffin added a commit that referenced this pull request Dec 4, 2023
Basic ruff formatting for journald source plugin

Signed-off-by: Thomas Sjögren <[email protected]>

---------

Signed-off-by: Thomas Sjögren <[email protected]>
Co-authored-by: Tom Tuffin <[email protected]>
Co-authored-by: Alex <[email protected]>
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.

4 participants