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

Fix too many open files error #285

Merged
merged 6 commits into from
Aug 21, 2024

Conversation

armallen
Copy link
Contributor

@armallen armallen commented Aug 19, 2024

Bugfix for #271
Opening all files at once does not seem necessary. fix_files already handles str file paths and opens them automatically.

Also,

  • maison 2.0.0 was released today and introduces breaking changes (maison.schema does not exist anymore), so dependency rules are updated to be a bit more restrictive. Updated to maison 2.0.0 will require a separate PR.
  • black is updated to >=24.3 to fix a vulnerability issue detected by safety
  • a .safety-policy.yml file is generated to ignore a jinja2 3.1.4 vulnerability that can probably be ignored since we don't use jinja2's from_string function?

Checklist

  • Add test cases to all the changes you introduce -> No new test needed
  • Update the documentation for the changes -> No update needed

@coveralls
Copy link

coveralls commented Aug 19, 2024

Pull Request Test Coverage Report for Build 10491853708

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 3 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.6%) to 98.984%

Files with Coverage Reduction New Missed Lines %
src/yamlfix/services.py 3 93.65%
Totals Coverage Status
Change from base Build 6968562450: -0.6%
Covered Lines: 487
Relevant Lines: 492

💛 - Coveralls

pyproject.toml Outdated
@@ -28,7 +28,7 @@ requires-python = ">=3.8"
dependencies = [
"click>=8.1.3",
"ruyaml>=0.91.0",
"maison>=1.4.0",
"maison>=1.4.0,<1.4.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sure we don't use neither maison 2.0.0 (breaks interface) nor maison 1.4.3 (which updates to pydantic 2.X and causes other incompatibilities with other dependencies).

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @armallen can you please either:

  • Fix the code to support maison > 2.0.0
  • Open an issue to support maison > 2.0.0 and add a comment above the line you edited in the PR?

Copy link

@gandhis1 gandhis1 Aug 23, 2024

Choose a reason for hiding this comment

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

It would have been ideal to have first released a version that pinned maison to <1.4.3 and still supported Python 3.8 before subsequently releasing a version that dropped support for Python 3.8 with the latest maison. This would have allowed people who are still on Python 3.8 to pin their yamlfix version. As it stands, I don't think any existing version of yamlfix will work with Python 3.8 because of the maison issue.

Is this still possible?

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @gandhis1, you are right, sorry for the inconveniences. I've created a new branch python3.8 and released version 1.16.1 with the fixed dependencies.

@armallen
Copy link
Contributor Author

@lyz-code do you have time to review this in the following weeks by any chance? 🙏

@lyz-code lyz-code mentioned this pull request Aug 20, 2024
2 tasks
Copy link
Owner

@lyz-code lyz-code left a comment

Choose a reason for hiding this comment

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

Thank you @armallen for your contribution and the clear comments in the PR (•‿•)

I do not like upper pinning dependencies without a reason, would you mind addressing the comment I've done?

Thank you

pyproject.toml Outdated
@@ -28,7 +28,7 @@ requires-python = ">=3.8"
dependencies = [
"click>=8.1.3",
"ruyaml>=0.91.0",
"maison>=1.4.0",
"maison>=1.4.0,<1.4.3",
Copy link
Owner

Choose a reason for hiding this comment

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

Hi @armallen can you please either:

  • Fix the code to support maison > 2.0.0
  • Open an issue to support maison > 2.0.0 and add a comment above the line you edited in the PR?

Copy link
Contributor Author

@armallen armallen left a comment

Choose a reason for hiding this comment

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

@lyz-code I went for the update to maison 2.0.0, which requires updating python min version to 3.9.1 and adding pydantic as a dependency.

pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
Copy link
Owner

@lyz-code lyz-code left a comment

Choose a reason for hiding this comment

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

Wow, awesome! thank you so much for the contribution

@lyz-code lyz-code merged commit 6ff6850 into lyz-code:main Aug 21, 2024
5 checks passed
@lyz-code
Copy link
Owner

Available since 1.17.0 thanks @armallen

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