8000 Add a `SocketPath` type for `linux` systems by theunkn0wn1 · Pull Request #10378 · pydantic/pydantic · GitHub
[go: up one dir, main page]
Skip to content

Add a SocketPath type for linux systems #10378

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 14, 2024
Merged

Add a SocketPath type for linux systems #10378

merged 1 commit into from
Sep 14, 2024

Conversation

theunkn0wn1
Copy link
Contributor
@theunkn0wn1 theunkn0wn1 commented Sep 10, 2024 •
edited by pydantic-hooky bot
Loading

Change Summary

The existing FilePath type does not accept sockets, as they fail the Path.is_file(self) check.
As such, I added a new SocketPath type and corresponding validator using Path.is_socket(self).

Related issue number

fix #10376

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Sep 10, 2024
@codspeed-hq CodSpeed HQ
Copy link
codspeed-hq bot commented Sep 10, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #10378 will not alter performance

Comparing theunkn0wn1:user/theunkn0wn1/validate_socket (b874a82) with main (7daa2cd)

Summary

✅ 49 untouched benchmarks

@theunkn0wn1
Copy link
Contributor Author

Test will need to be skipped on windows; as it doesn't have sockets to begin with.
Not sure why the tests are failing on mac.

Viicos
Viicos reviewed Sep 12, 2024
View reviewed changes
Copy link
Member
@Viicos Viicos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering if we should do something like:

Edit: Seems like on Windows, is_socket returns False, so maybe it's safe to not add these kind of conditions.

@Viicos Viicos changed the title Add a SocketPath type Add a SocketPath type Sep 12, 2024
@sydney-runkle
Copy link
Contributor

Looking good though, happy to support this once we get tests passing, etc :).

@theunkn0wn1
Copy link
Contributor Author

ok for the issue with the Mac tests - pytest seems to be configured wrong, the temporary paths its generating with its fixture exceed the maximum length of a socket name on that platform.
Only way i can see to fix that would be to change the pytest config to use a more sensible base directory.
the trouble? i don't have a mac to test with and don't know what a sensible base path for that platform is.
Can I ask for some pointers here?

@Viicos
Copy link
Member
Viicos commented Sep 13, 2024

ok for the issue with the Mac tests - pytest seems to be configured wrong, the temporary paths its generating with its fixture exceed the maximum length of a socket name on that platform.
Only way i can see to fix that would be to change the pytest config to use a more sensible base directory.
the trouble? i don't have a mac to test with and don't know what a sensible base path for that platform is.
Can I ask for some pointers here?

I'm ok only running the test on Linux. The goal isn't to test that creating sockets works, but to ensure that validate_socket works as expected when encountering a socket path.

@theunkn0wn1 theunkn0wn1 force-pushed the user/theunkn0wn1/validate_socket branch 2 times, most recently from 62fdf62 to 10d4ce8 Compare September 13, 2024 19:58
@theunkn0wn1 theunkn0wn1 force-pushed the user/theunkn0wn1/validate_socket branch from 10d4ce8 to b874a82 Compare September 13, 2024 20:03
@theunkn0wn1
Copy link
Contributor Author

Added condition to skip new test if not linux.
squashed commits to one.
rebased against tip of upstream main. :)

@github-actions GitHub Actions
Copy link
Contributor

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  pydantic
  types.py 1277
Project Total  

This report was generated by python-coverage-comment-action

@theunkn0wn1
Copy link
Contributor Author

please review :)

sydney-runkle
sydney-runkle approved these changes Sep 14, 2024
View reviewed changes
Copy link
Contributor
@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@sydney-runkle sydney-runkle changed the title Add a SocketPath type Add a SocketPath type for linux systems Sep 14, 2024
@sydney-runkle sydney-runkle merged commit 5d5b8af into pydantic:main Sep 14, 2024
59 checks passed
@theunkn0wn1 theunkn0wn1 deleted the user/theunkn0wn1/validate_socket branch September 24, 2024 18:12
@theunkn0wn1
Copy link
Contributor Author

@sydney-runkle when can i expect this change to be released? 😇

@sydney-runkle
Copy link
Contributor
sydney-runkle comme 9056 nted Sep 25, 2024

@theunkn0wn1, with our 2.10 release in late October! Feel free to install from main in the meantime :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Viicos Viicos Viicos left review comments

@sydney-runkle sydney-runkle sydney-runkle approved these changes

Assignees

@sydney-runkle sydney-runkle

Labels
relnotes-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pydantic FilePath does not accept sockets
3 participants
@theunkn0wn1 @sydney-runkle @Viicos
0