-
Notifications
You must be signed in to change notification settings - Fork 127
Adding dropdown to select the Python version in the sandbox #670
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
I see an unrelated compilation issue/error in dataclass.rs in the CI/CD tests, is this expected? |
Yeah, the build is currently broken - sorry! A fix is being landed and will hopefully be exported to GitHub shortly. |
Should be fixed by 1546fa9. |
0a25a56
to
eb33b41
Compare
ci-shell-test-jest is failing. Looking :) |
import React from 'react'; | ||
import * as stylex from '@stylexjs/stylex'; | ||
|
||
const SUPPORTED_VERSIONS = ['3.8', '3.9', '3.10', '3.11', '3.12'] as const; |
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 include 3.13 and 3.14 here?
If I understand correctly, we aren't changing the interpreter version here (which is reasonable, although we need to make sure the UI is clear on this), in which case we might as well use an expansive set of versions, which more easily allows testing newer typeshed features
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 can edit. CI is still red on one of the jobs though with a "null pointer passed to rust" error message. I could not repro locally cc'd David in case there are additional thoughts on how to repro
…stract methods. Updated the `AnswersSolver` to handle calls to abstract methods, raising an `AbstractMethodCall` error when such methods are invoked directly.
Hi! Sorry for the delay on this review, I looked into the CI error being red and this was the error:
We have a test which checks some of the wasm behavior, and we need to update the constructor there. Here's the code pointer: https://github.com/facebook/pyrefly/blob/main/website/src/__tests__/__test_utils__/PyreflyWasmTestUtils.ts#L35, we can default this to a hardcoded python version for now (probably 3.12) |
Giving this a shot |
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.
Overall code looks good to me, mainly have feedback on the UI. Feel free to also break this up if you would want to commit the wasm portion first (client can just hardcode to a default python version for now)
export interface PythonVersionSelectorProps { | ||
selectedVersion: string; | ||
onVersionChange: (version: string) => void; | ||
disabled?: boolean; |
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: call this loading, and we can probably just make this non-optional
{!isCodeSnippet && ( | ||
<div {...stylex.props(styles.versionSelectorContainer)}> | ||
<PythonVersionSelector | ||
selectedVersion={pythonVersion} | ||
onVersionChange={handleVersionChange} | ||
disabled={loading} | ||
/> | ||
</div> | ||
)} |
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 actually move the version selector into the buttonContainerBase
, we may want to rename that to reflect this change. The main reason is that the current UI looks a bit odd with the selector being in a different place from the buttons, especially on mobile. I think let's try to move it and see if we can make the UI consistent with the buttons as well.
Here's what I see right now of how it looks on mobile + desktop
<select | ||
{...stylex.props(styles.versionSelector)} | ||
value={selectedVersion} | ||
onChange={(e) => onVersionChange(e.target.value)} | ||
disabled={disabled} | ||
aria-label="Select Python version" | ||
> | ||
{SUPPORTED_VERSIONS.map((version) => ( | ||
<option key={version} value={version}> | ||
Python {version} | ||
</option> | ||
))} | ||
</select> | ||
); |
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.
@migeed-z Thanks for helping out! Let me know if you want me to take a look at anything, I can take over as well. |
can you apply David's comments and then I will be happy to import it and look into it again :) |
Addresses #582. Adds the ability to test different Python versions.
This PR implements Python version selection in the sandbox, allowing users to test their code against different Python versions (3.8-3.12) and see how type checking behavior differs across versions.