Adds validation: parameter name must be specified if value is specified #65
Conversation
|
|
|
@jweill-aws |
src/components/parameters-picker.tsx
Outdated
| const useForceUpdate = () => { | ||
| const [, setTick] = useState(0); | ||
| const update = useCallback(() => { | ||
| setTick(tick => tick + 1); | ||
| }, []); | ||
| return update; | ||
| }; |
There was a problem hiding this comment.
Without useForceUpdate, although the model is updated, the parameters component does not automatically rerender. This function is meant to update a nominal state to force a rerender.
There was a problem hiding this comment.
Do we know why this behavior is happening?
That seems like a hack to support a refresh that React supports by design; this is likely an issue with our code. If ParametersPicker is a functional component, any update to props is supposed to re-render it. If you need internal state in the ParametersPicker to trigger changes, then that's the way to go. In either case, there shouldn't be a pattern of adding an unused state to force updates.
There was a problem hiding this comment.
A parameter with a name and an empty-string value is not inherently an error; there might be a reasonable reason for that. (The
errorsvariable uses an empty-string value to mean "no error" for a particular input name, for example.) @ellisonbg had suggested the specific case of "empty parameter name, non-empty parameter value" being an error, but I don't consider the other three cases to be problematic:Empty name Name
Empty value ✅ ✅
Value ❌ ✅
From a user's perspective, there is a very low chance they would want to pass an empty string for a parameter; for other cases (number, boolean) this will cause an exception. On the other hand, blocking users to pass an empty value will hinder some users, I don't feel that we will have a lot of users who would want this. Ok to submit this as-is and revisit after some user feedback.
There was a problem hiding this comment.
BTW, the async update was fixed in #63.
A parameter with a name and an empty-string value is not inherently an error; there might be a reasonable reason for that. (The
|
I believe this is to preserve parity with shell environment variables, which can be defined without values. A common example is just to define |
|
Let me rebase off of |
|
Other bugs I found while playing around: Screen.Recording.2022-09-30.at.10.44.14.AM.movDeleting param should clear the corresponding errors. Screen.Recording.2022-09-30.at.10.47.03.AM.movError state is not being correctly re-calculated upon deleting a param in the middle. |
dlqqq
force-pushed
the
parameter-validation
branch
from
f5187bf
to
4883032
Compare
|
@jweill-aws @3coins I rebased off of |
updates: - [github.com/pre-commit/pre-commit-hooks: v4.2.0 → v4.3.0](pre-commit/pre-commit-hooks@v4.2.0...v4.3.0) - [github.com/psf/black: 22.3.0 → 22.8.0](psf/black@22.3.0...22.8.0) - [github.com/asottile/pyupgrade: v2.32.1 → v2.38.2](asottile/pyupgrade@v2.32.1...v2.38.2) - [github.com/pycqa/flake8: 4.0.1 → 5.0.4](PyCQA/flake8@4.0.1...5.0.4) - [github.com/sirosen/check-jsonschema: 0.15.0 → 0.18.3](python-jsonschema/check-jsonschema@0.15.0...0.18.3)
…erver#61) * Improvements to the MUI theme for JupyterLab. * Additional theme improvements. Co-authored-by: Brian E. Granger <brgrange@amazon.com>
In the create form, if the parameter value is nonblank, the parameter name must not be blank.
If the parameter name and value are both blank, there is no error.
If the parameter name is nonblank and the value is blank, there is no error.
Errors are displayed on name or value blur.
There is no validation for the value.
parameter-validation.mov