Skip to content

Conversation

piyush588
Copy link
Contributor

@piyush588 piyush588 commented Jul 23, 2025

Description:
This PR introduces a GitHub Actions workflow that runs Pylint on only the Python files changed in a pull request.

What the Workflow Does
Triggers only on pull requests targeting the main branch

Detects .py files changed in the PR compared to main

Runs Pylint only on those files

Ignores the following warnings to reduce noise:

W0212: Access to a protected member

E0611: No name in module

E0401: Import error

Related issue(s):

Fixes #190

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@piyush588 piyush588 changed the title Feat: add pylint github action feat: add pylint github action Jul 23, 2025
@nadineloepfe
Copy link
Contributor

Amazing PR!
However, I think it would be better to have a file ci.yml and to merge this one here with the test one. so first we would run the lint job and then run unit and integration tests.

By having a multi job pipeline, we can comprehensive CI that validates code quality and correctness on every pull request

@exploreriii

This comment was marked as resolved.

@exploreriii
Copy link
Contributor

To add to this @piyush588 @nadineloepfe, I've been doing some type hinting and some of the errors can (not very often) be quite varied and sometimes are based on historical style choices. I would propose just doing a pylint and mypy 'check' but not a requirement to for all to pass to approve, at least until we have completed more linting/type code improvements.

@exploreriii

This comment was marked as resolved.

@aceppaluni
Copy link
Contributor

Hi all, I took a look at this from the meeting this morning and would be happy to help.
@piyush588 if you need assistance please let me know, I have a bit of an idea on how this can be done.

@piyush588
Copy link
Contributor Author

Hi @aceppaluni ,
Thankyou so much! I’d really appreciate your help I was a bit stuck with the last part, especially implementing Nadine’s suggestions correctly.
If you already have an idea, I’d love to hear your thoughts and maybe we can work through it together. Let me know how you’d like to proceed!

@aceppaluni
Copy link
Contributor

Hi @piyush588 Of course! I sent you a message in discord :)

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

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

Thanks. Sorry it took me a while to review this - I had to do some research.
I think this is mostly ready to go, we should add: continue-on-error: true
until we have refined our linting a bit more.

@nadineloepfe, I would suggest creating an integrated workflow in an additional issue as we have other workflows coming in, some may not be necessary to merge, and this could require more research

Copy link
Contributor

@nadineloepfe nadineloepfe left a comment

Choose a reason for hiding this comment

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

I've left some comments to make the pipeline more robust and easier to read - please let me know if you have any questions!

with:
python-version: ${{ matrix.python-version }}

- name: Install dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Install Pylint

run: |
python -m pip install --upgrade pip
pip install pylint
# Running pylint on pull requests while ignoring:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the comments here

run: |
git fetch origin main
changed_files=$(git diff --name-only origin/main...HEAD | grep '\.py$' || true)
echo "Changed Python files:"
Copy link
Contributor

Choose a reason for hiding this comment

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

change to "Linting the following files" for clarity

runs-on: ubuntu-latest
strategy:
matrix:
python-version: ["3.10"]
Copy link
Contributor

Choose a reason for hiding this comment

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

can be removed as we just setup python version once, so this can just be a step like " name: Set up Python 3.10.."


- name: Install dependencies
run: |
python -m pip install --upgrade pip
Copy link
Contributor

Choose a reason for hiding this comment

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

can we stick to uv for the entire workflow? else we're mixing uv and pip

@piyush588
Copy link
Contributor Author

@exploreriii
Could you help me understand the error "CHANGELOG.md was not changed in this PR"?
How do I properly add an entry to CHANGELOG.md?

Also, I’m seeing the message:
"This branch has conflicts that must be resolved .github/workflows/test.yml"
Do you know why this might be happening?

@exploreriii
Copy link
Contributor

CHANGELOG.md... this is in the project root, find the file and in the added section just add eg
"added a Pylint check for pull requests"

Yes, you have a conflict because your branch is not as updated as main. Main just changed, specifically the file test.yml. That means your branch thinks test.yml should be version B, but main thinks it should be version A. When you merge your project (B) into main (A), this causes the issue: which file should be the outcome: the main's version (A) or your outcome (B)?

So you first need to update your branch with the recent changes from main
This means you'll need to decide what to do with the test.yml
Since test.yml has changed, you might have to change your approach

Review README_upstream for rebase instructions
https://github.com/hiero-ledger/hiero-sdk-python/pull/239/files#diff-a3202b0afcb1aff9b560a5e664ddcab1fe31deb7568ef535156fe9dbfd7c960b

run: uv pip install -e .

- name: Run integration tests
run: uv run pytest -m integration
Copy link
Contributor

Choose a reason for hiding this comment

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

- name: Run integration tests
        env:
          OPERATOR_ID: ${{ steps.solo.outputs.accountId }}
          OPERATOR_KEY: ${{ steps.solo.outputs.privateKey }}
          ADMIN_KEY: ${{ steps.solo.outputs.privateKey }}
          PUBLIC_KEY: ${{ steps.solo.outputs.publicKey }}
          NETWORK: solo
        run: |
          uv run pytest -m integration

needs to be changed back to this or the CI will fail

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

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

We have a new release so please change your changelog, ensure it is not under added for v0.1.4, it should be in the unreleased added (which will be for v0.1.5)

@piyush588 piyush588 requested a review from exploreriii August 26, 2025 04:24
Copy link
Contributor Author

@piyush588 piyush588 left a comment

Choose a reason for hiding this comment

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

Updated the ci.yml

@exploreriii
Copy link
Contributor

Seen
Sorry this is taking a while, I have to research some things

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

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

Unfortunately this will fail but your code is good!
The issue is we discovered 3.10 unit tests are failing, so pinning it to 3.10 will cause a failure

We have an issue open to fix the comp issues, in which case I would recommend either setting it to a general version that uv will select:

  • name: Setup Python
    run: uv python install

or, waiting until 3.10 is fixed and then proceed - but we should also make it run for 3.10, 3.11, 3.12 (which is one of the open issues)

In which case, i would recommend generalising it for now and we will add to it in the future as we have open issues

@exploreriii
Copy link
Contributor

also please rebase

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.

Research Process to add to Github Actions: Pylint
4 participants