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

linting: Always use latest versions of linting tools; Upgrade clang-format config. #61

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

LinZhihao-723
Copy link
Member

References

Description

The current code base was formatted by out-dated linting tools. When running gh workflow, linting check will fail. This PR has the following updates:

  • Update all linting tools to the latest pypi released versions.
  • Update clang-format config with new features added in version 18. In addition, format yaml file to match CLP core style. Please check this PR for more details: linting: Always use latest versions of linting tools; Upgrade clang-format config. clp#384
  • Refactor everything after upgrading linter versions.
    Note: ideally, we can drop the versions specified in requirements-dev.txt to ensure it always uses the latest one. However, adding the version there would help us keep track of which version of the config file we used to setup these linting tools.

Validation performed

  • Ensured workflow passed.

@davemarco
Copy link

I ran tests and linters with no errors.

@davemarco
Copy link

It may make sense to add -U to pip install -r requirements-dev.txt in the README so devs upgrade to latest version of linter

@LinZhihao-723
Copy link
Member Author

It may make sense to add -U to pip install -r requirements-dev.txt in the README so devs upgrade to latest version of linter

It is expected to run with a virtual env so upgrade is not necessarily needed

@davemarco
Copy link

I know clp uses poetry, but not sure if the github action linter uses the lock file from the package. It is probably overkill for this repo. I am okay with as is

@davemarco
Copy link

It may make sense to add -U to pip install -r requirements-dev.txt in the README so devs upgrade to latest version of linter

It is expected to run with a virtual env so upgrade is not necessarily needed

Makes sense

@LinZhihao-723
Copy link
Member Author

I know clp uses poetry, but not sure if the github action linter uses the lock file from the package. It is probably overkill for this repo. I am okay with as is

Sorry I didn't quite get this comment...

@davemarco
Copy link

Sorry I didn't quite get this comment...

I know clp uses poetry, but not sure if the github action linter uses the lock file from the package. It is probably overkill for this repo. I am okay with as is

Sorry I didn't quite get this comment...

just bringing the linters will auto update for the action, which could cause the workflow to fail even if it ran fine locally

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

Can we use the same PR title we used in y-scope/clp?

@LinZhihao-723
Copy link
Member Author

Can we use the same PR title we used in y-scope/clp?

sure

@kirkrodrigues kirkrodrigues changed the title linting: Upgrade linting tools to the latest PyPI release versions; Upgrade clang-format config. linting: Always use latest versions of linting tools; Upgrade clang-format config. Jun 4, 2024
@kirkrodrigues kirkrodrigues merged commit 37b2ae9 into y-scope:main Jun 4, 2024
60 checks passed
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.

3 participants