-
Couldn't load subscription status.
- Fork 39
Add Python package #87
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
base: main
Are you sure you want to change the base?
Conversation
|
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D84891338. (Because this pull request was imported automatically, there will not be any future comments.) |
I have accepted this invitation, but I won't complete the ownership transfer until this gets merged in. I am pretty happy with how this looks at a quick glance, but I will do a more thorough review next week. I will probably open a PR on your fork to address some of the devcontainer stuff I see that should be addressed with this as well. |
|
Thanks, sounds good! I pushed some final cleanup changes like making sure the licenses are shipped with each wheel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
back to you for mostly minor stuff. This is great, and thanks for putting it together!
| @@ -0,0 +1,159 @@ | |||
| name: publish downstream packages | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you include the --- to indicate the start of a yaml doc please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do this in a separate PR since the other CI files don't have this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just get right for the new files; you don't need to update other stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
| >>> import dotslash | ||
| >>> dotslash.locate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make a pytest test that does this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added tests.
| The installed DotSlash binary can be invoked directly by running the `dotslash` module as a script. | ||
|
|
||
| ``` | ||
| python -m dotslash path/to/dotslash-file.json | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add a new python-test command in the Justfile and then run that in our CI to verify this continues to work as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this and also added a line to make it work on Windows for me.
| line-length = 120 | ||
|
|
||
| [lint.flake8-tidy-imports] | ||
| ban-relative-imports = "all" | ||
|
|
||
| [lint.isort] | ||
| known-first-party = ["dotslash"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoyingly, we don't seem to have a global ruff.toml file that I can just have included by default. Internal linters are not upset with the formatting here, so it's fine, but something for me to look into in the future I guess -_-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should enable ufmt: https://ufmt.omnilib.dev/en/stable/
(unfortunately, private wiki links follow)
@sdwilsh see: https://www.internalfb.com/wiki/Python/code_formatting/ruff/
We should follow this wiki: https://www.internalfb.com/wiki/Python/code_formatting/pyfmt/#replicating-pyfmt-in-ope
Which will require updating the imported Diff a bit. @sdwilsh do you have capacity to go do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Ruff unsupported internally i.e. you must use ufmt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this makes sense. All the ruff things I saw were for other open source projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will see if I can get this working today. I'm unfortunately off tomorrow, so if it doesn't happen today, I'll get it resolved on Monday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like ufmt can use ruff, if I'm reading the internal wiki properly, so I will see about making this work today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable, but I think we need to reflect pyfmt correctly. I don't really have capacity right now but Shawn might
(sorry for how long it took to review this PR)
| line-length = 120 | ||
|
|
||
| [lint.flake8-tidy-imports] | ||
| ban-relative-imports = "all" | ||
|
|
||
| [lint.isort] | ||
| known-first-party = ["dotslash"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should enable ufmt: https://ufmt.omnilib.dev/en/stable/
(unfortunately, private wiki links follow)
@sdwilsh see: https://www.internalfb.com/wiki/Python/code_formatting/ruff/
We should follow this wiki: https://www.internalfb.com/wiki/Python/code_formatting/pyfmt/#replicating-pyfmt-in-ope
Which will require updating the imported Diff a bit. @sdwilsh do you have capacity to go do that?
|
Okay, I think I addressed everything now! The only exception is the Ruff/ufmt feedback but as you know I don't have access to the documentation on what needs doing. |
|
@sdwilsh can you re-review? And message me internally about what to do about the formatter stuff? |
Hello again! This is the second half of what we require in order to start using DotSlash.
This moves over the code for the
dotslashPython package I created over the weekend. The ownership transfer on PyPI is in progress.The package serves a similar purpose to the existing
fb-dotslashNode.js package stored in the top levelnodedirectory. Some notes:locatefunction for finding the path to the binary that was installed by the installation. The logic was adapted from equivalents like UV's finder. I optimized the path resolution to perform as little work as necessary and to do so lazily.python -m dotslash. If there ever is a desire to represent paths as anything other than strings in the implementation, please don't.DOTSLASH_VERSIONenvironment variable to the desired release tag of DotSlash (thevis optional). This will be used as the package version and, by default, for fetching the appropriate release artifact.DOTSLASH_SOURCEenvironment variable to the path to a directory containing the release artifacts. I strongly recommend that the release process uses that eventually (and the Node.js package's build script gains support).release-downstream.ymlworkflow from this repository. You might find hazy details about their support for reusable workflows but this comment provides the full picture. Basically, the claim they chose happens to work for the most nested called workflow when using reusable workflows but not the top level one. Whenever support for that happens there will be no user impact for existing supported scenarios.cc @sdwilsh @bigfootjon for review as requested
Also, it's worth noting that PyPI was blocking the name
dotslashdue to certain rules and @di very graciously provided the override 🙇♂️