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

feat(ruff): add option for ruff as linter and auto-formatter #298

Merged
merged 6 commits into from
Jun 26, 2024

Conversation

Naman-Priyadarshi
Copy link
Contributor

@Naman-Priyadarshi Naman-Priyadarshi commented Jun 16, 2024

Pull Request description

This pull request introduces two separate configuration options for ruff: linter and auto-formatter. These changes enhance the flexibility of our code quality tools, allowing users to choose whether they want to use Ruff solely as a linter, solely as an auto-formatter, or both.

Also added both the configurations to the osl profile and base profile.

WIP: Adding documentation to guide.md

Fixes #220

How to test these changes

makim tests.unittest
or
create a project manually

Pull Request checklists

This PR is a:

  • bug-fix
  • new feature
  • maintenance

About this PR:

  • it includes tests.
  • the tests are executed on CI.
  • the tests generate log file(s) (path).
  • pre-commit hooks were executed locally.
  • this PR requires a project documentation update.

Author's checklist:

  • I have reviewed the changes and it contains no misspelling.
  • The code is well commented, especially in the parts that contain more
    complexity.
  • New and old tests passed locally.

Additional information

Reviewer's checklist

Copy and paste this template for your review's note:

## Reviewer's Checklist

- [ ] I managed to reproduce the problem locally from the `main` branch
- [ ] I managed to test the new changes locally
- [ ] I confirm that the issues mentioned were fixed/resolved

Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

thanks for working on that @Naman-Priyadarshi

@@ -48,7 +48,7 @@ sort_by_size = true
verbose = false
{% endif -%}

{%- if cookiecutter.use_ruff == "yes" %}
{%- if cookiecutter.use_ruff_linter == "yes" %}
Copy link
Member

Choose a reason for hiding this comment

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

no needs for keeping different if for ruff linter and ruff formatter

just use something like:

if cookiecutter.use_ruff_linter == "yes" or cookiecutter.use_ruff_formatter == "yes"

or maybe something like (not really sure if that works inside a jinja2 tag):

`if "yes" in [cookiecutter.use_ruff_linter, cookiecutter.use_ruff_formatter]`

@xmnlab
Copy link
Member

xmnlab commented Jun 18, 2024

Hey @Naman-Priyadarshi , it seems that there is 1 file that needs an format update, according to ruff formatter ... let me know if you need any help to identify what need to be done

@Naman-Priyadarshi
Copy link
Contributor Author

Naman-Priyadarshi commented Jun 18, 2024

I tried running pre-commit run --all-files --verbose and received the output that All checks passed!. Is there any other command I should use or find it manually in the changed files?

@xmnlab
Copy link
Member

xmnlab commented Jun 18, 2024

diff --git a/src/osl_python_package/__init__.py b/src/osl_python_package/__init__.py
index 87fa060..d5d5599 100644
--- a/src/osl_python_package/__init__.py
+++ b/src/osl_python_package/__init__.py
@@ -8,11 +8,11 @@ def get_version() -> str:
     try:
         return importlib_metadata.version(__name__)
     except importlib_metadata.PackageNotFoundError:  # pragma: no cover
-        return '0.1.0'  # semantic-release
+        return "0.1.0"  # semantic-release
 
 
 version = get_version()
 
 __version__ = version

@@ -48,7 +48,7 @@ sort_by_size = true
verbose = false
{% endif -%}

{%- if cookiecutter.use_ruff == "yes" %}
{%- if cookiecutter.use_ruff_linter == "yes" or cookiecutter.use_ruff_formatter == "yes" %}
Copy link
Member

Choose a reason for hiding this comment

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

you can add at the end of the tool.ruff tables/sections

https://github.com/arxlang/astx/blob/main/pyproject.toml#L100-L101

[tool.ruff.format]
quote-style = "double"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried both single and double, still getting the same error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @xmnlab, since the "double" format did not work, maybe we would change the generation of
return '0.1.0' # semantic-release to use double quotes instead?

@Naman-Priyadarshi
Copy link
Contributor Author

Hi @xmnlab, the test seems to be failing due to some miscellaneous error

  at ~/miniconda3/envs/osl-python-package/lib/python3.12/site-packages/poetry/utils/env/base_env.py:342 in _run
      338│                 output = subprocess.check_output(
      339│                     cmd, stderr=stderr, env=env, text=True, **kwargs
      340│                 )
      341│         except CalledProcessError as e:
    → 342│             raise EnvCommandError(e)
      343│ 
      344│         return output
      345│ 
      346│     def execute(self, bin: str, *args: str, **kwargs: Any) -> int:

Cannot install rapidfuzz.

Could you help me out here? Thank you!

@xmnlab
Copy link
Member

xmnlab commented Jun 26, 2024

Hi @xmnlab, the test seems to be failing due to some miscellaneous error

  at ~/miniconda3/envs/osl-python-package/lib/python3.12/site-packages/poetry/utils/env/base_env.py:342 in _run
      338│                 output = subprocess.check_output(
      339│                     cmd, stderr=stderr, env=env, text=True, **kwargs
      340│                 )
      341│         except CalledProcessError as e:
    → 342│             raise EnvCommandError(e)
      343│ 
      344│         return output
      345│ 
      346│     def execute(self, bin: str, *args: str, **kwargs: Any) -> int:

Cannot install rapidfuzz.

Could you help me out here? Thank you!

hi @Naman-Priyadarshi ! when things weird like that happen in CI, just restart that to check if it is some random issue in the installation ... usually that fix the problem.

if it is something about distlib ... maybe it is some conflict between conda and poetry .. so usually I just add that dep inside the conda environment file in the pip section ... usually that fix the issue

in this case I just restarted the CI and everything is working

@xmnlab
Copy link
Member

xmnlab commented Jun 26, 2024

thanks for working on that @Naman-Priyadarshi

@xmnlab xmnlab merged commit 549bd67 into osl-incubator:main Jun 26, 2024
16 checks passed
@Naman-Priyadarshi
Copy link
Contributor Author

Hi @xmnlab, the test seems to be failing due to some miscellaneous error

at ~/miniconda3/envs/osl-python-package/lib/python3.12/site-packages/poetry/utils/env/base_env.py:342 in _run

  338│                 output = subprocess.check_output(
  339│                     cmd, stderr=stderr, env=env, text=True, **kwargs
  340│                 )
  341│         except CalledProcessError as e:
→ 342│             raise EnvCommandError(e)
  343│ 
  344│         return output
  345│ 
  346│     def execute(self, bin: str, *args: str, **kwargs: Any) -> int:

Cannot install rapidfuzz.

Could you help me out here? Thank you!

hi @Naman-Priyadarshi ! when things weird like that happen in CI, just restart that to check if it is some random issue in the installation ... usually that fix the problem.

if it is something about distlib ... maybe it is some conflict between conda and poetry .. so usually I just add that dep inside the conda environment file in the pip section ... usually that fix the issue

in this case I just restarted the CI and everything is working

yeah that makes complete sense, the problem is that maybe I don't have permissions to restart the ci! that's why I mentioned you to do it for me hahaha, anyways thank you 😁

@Naman-Priyadarshi Naman-Priyadarshi deleted the ruff-auto branch July 8, 2024 15:53
Copy link

🎉 This issue has been resolved in version 0.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

template: add ruff as auto-formatter tool
2 participants