You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tooling.apache.org by "andrewmusselman (via GitHub)" <gi...@apache.org> on 2025/11/20 23:16:06 UTC
[PR] Make the draft forms more type safe (tooling-trusted-releases)
andrewmusselman opened a new pull request, #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336
Tested making and deleting a draft, added a js validation on delete draft field and button since I don't see how that was done before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Make the draft forms more type safe (tooling-trusted-releases)
Posted by "andrewmusselman (via GitHub)" <gi...@apache.org>.
andrewmusselman commented on code in PR #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336#discussion_r2594669853
##########
atr/post/draft.py:
##########
@@ -186,11 +159,11 @@ async def hashgen(session: web.Committer, project_name: str, version_name: str,
@post.committer("/draft/sbomgen/<project_name>/<version_name>/<path:file_path>")
+@post.empty()
async def sbomgen(session: web.Committer, project_name: str, version_name: str, file_path: str) -> web.WerkzeugResponse:
"""Generate a CycloneDX SBOM file for a candidate draft file, creating a new revision."""
await session.check_access(project_name)
- await util.validate_empty_form()
Review Comment:
@sbp I remember taking some validation out because things are validated elsewhere, checking this is correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
Re: [PR] Make the draft forms more type safe (tooling-trusted-releases)
Posted by "sbp (via GitHub)" <gi...@apache.org>.
sbp merged PR #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Make the draft forms more type safe (tooling-trusted-releases)
Posted by "sbp (via GitHub)" <gi...@apache.org>.
sbp commented on code in PR #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336#discussion_r2589221044
##########
atr/post/draft.py:
##########
@@ -37,37 +37,18 @@
import atr.web as web
-class VotePreviewForm(forms.Typed):
- body = forms.textarea("Body")
- # TODO: Validate the vote duration again?
- # Probably not necessary in a preview
- # Note that tasks/vote.py does not use this form
- vote_duration = forms.integer("Vote duration")
+class VotePreviewForm(form.Form):
+ body: str = form.label("Body", widget=form.Widget.TEXTAREA)
+ # Note: this does not provide any vote duration validation; this simply displays a preview to the user
+ vote_duration: form.Int = form.label("Vote duration")
-@post.committer("/draft/delete")
-async def delete(session: web.Committer) -> web.WerkzeugResponse:
+@post.committer("/compose/<project_name>/<version_name>")
+@post.form(shared.draft.DeleteForm)
+async def delete(
+ session: web.Committer, form: shared.draft.DeleteForm, project_name: str, version_name: str
Review Comment:
Let's call this `delete_form` so as not to look like `atr.form`.
##########
atr/post/draft.py:
##########
@@ -92,19 +73,14 @@ async def delete(session: web.Committer) -> web.WerkzeugResponse:
@post.committer("/draft/delete-file/<project_name>/<version_name>")
-async def delete_file(session: web.Committer, project_name: str, version_name: str) -> web.WerkzeugResponse:
+@post.form(shared.draft.DeleteFileForm)
+async def delete_file(
+ session: web.Committer, form: shared.draft.DeleteFileForm, project_name: str, version_name: str
Review Comment:
Let's call this `delete_file_form`.
##########
atr/post/draft.py:
##########
@@ -243,25 +219,21 @@ async def sbomgen(session: web.Committer, project_name: str, version_name: str,
@post.committer("/draft/vote/preview/<project_name>/<version_name>")
+@post.form(VotePreviewForm)
async def vote_preview(
- session: web.Committer, project_name: str, version_name: str
+ session: web.Committer, form: VotePreviewForm, project_name: str, version_name: str
Review Comment:
Let's call this `vote_preview_form`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
Re: [PR] Make the draft forms more type safe (tooling-trusted-releases)
Posted by "sbp (via GitHub)" <gi...@apache.org>.
sbp commented on PR #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336#issuecomment-3613162912
Test comment, `Thu Dec 4 16:41:56 GMT 2025`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Make the draft forms more type safe (tooling-trusted-releases)
Posted by "andrewmusselman (via GitHub)" <gi...@apache.org>.
andrewmusselman commented on code in PR #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336#discussion_r2551310445
##########
atr/templates/check-selected.html:
##########
@@ -213,5 +184,21 @@ <h3 id="delete-draft" class="mt-4">Delete this draft</h3>
{{ super() }}
<script>
init();
+
+ // Delete form validation
Review Comment:
@sbp [a0851b0](https://github.com/apache/tooling-trusted-releases/pull/336/commits/a0851b06dd163bf0423440b4547e8ac822c3ccaf) fixes this; please review, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Make the draft forms more type safe (tooling-trusted-releases)
Posted by "sbp (via GitHub)" <gi...@apache.org>.
sbp commented on code in PR #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336#discussion_r2557642441
##########
atr/post/draft.py:
##########
@@ -37,37 +36,12 @@
import atr.web as web
-class VotePreviewForm(forms.Typed):
- body = forms.textarea("Body")
- # TODO: Validate the vote duration again?
Review Comment:
This comment was lost in the new class. Is the comment, or any of its three lines, important? Should it have been retained, in part or in whole?
##########
atr/get/draft.py:
##########
@@ -44,7 +44,6 @@ async def tools(session: web.Committer, project_name: str, version_name: str, fi
modified = int(await aiofiles.os.path.getmtime(full_path))
file_size = await aiofiles.os.path.getsize(full_path)
-
Review Comment:
Formatting changes should be reserved for a separate PR. We prefer minimal PRs because that makes them easier to review.
##########
atr/shared/__init__.py:
##########
@@ -21,10 +21,12 @@
import atr.db as db
import atr.db.interaction as interaction
-import atr.forms as forms
+import atr.form as atr_form
Review Comment:
We don't change interface names when we import them.
##########
atr/shared/draft.py:
##########
@@ -15,21 +15,29 @@
# specific language governing permissions and limitations
# under the License.
-import atr.forms as forms
+import pydantic
+import atr.form as form
-class DeleteFileForm(forms.Typed):
- """Form for deleting a file."""
- file_path = forms.string("File path")
- submit = forms.submit("Delete file")
+class DeleteFileForm(form.Form):
+ file_path: str = form.label(description="File path")
-class DeleteForm(forms.Typed):
- """Form for deleting a candidate draft."""
+class DeleteForm(form.Form):
+ release_name: str = form.label(description="Release name", widget=form.Widget.HIDDEN)
Review Comment:
The `description=` keyword should be removed from all of these `label` calls.
##########
atr/shared/draft.py:
##########
@@ -15,21 +15,29 @@
# specific language governing permissions and limitations
# under the License.
-import atr.forms as forms
+import pydantic
+import atr.form as form
-class DeleteFileForm(forms.Typed):
- """Form for deleting a file."""
- file_path = forms.string("File path")
- submit = forms.submit("Delete file")
+class DeleteFileForm(form.Form):
+ file_path: str = form.label(description="File path")
Review Comment:
We don't use keywords for the arguments to `form.label`. As mentioned on my recent #309 review, the general pattern here is that we don't use keywords for arguments before `*`, but that might not apply globally. We should probably check.
##########
atr/shared/draft.py:
##########
@@ -15,21 +15,29 @@
# specific language governing permissions and limitations
# under the License.
-import atr.forms as forms
+import pydantic
+import atr.form as form
-class DeleteFileForm(forms.Typed):
- """Form for deleting a file."""
- file_path = forms.string("File path")
- submit = forms.submit("Delete file")
+class DeleteFileForm(form.Form):
+ file_path: str = form.label(description="File path")
-class DeleteForm(forms.Typed):
- """Form for deleting a candidate draft."""
+class DeleteForm(form.Form):
+ release_name: str = form.label(description="Release name", widget=form.Widget.HIDDEN)
+ project_name: str = form.label(description="Project name", widget=form.Widget.HIDDEN)
+ version_name: str = form.label(description="Version name", widget=form.Widget.HIDDEN)
+ confirm_delete: str = form.label(description="Type DELETE to confirm")
- release_name = forms.hidden()
- project_name = forms.hidden()
- version_name = forms.hidden()
- confirm_delete = forms.string("Confirmation", validators=forms.constant("DELETE"))
- submit = forms.submit("Delete candidate draft")
+ @pydantic.field_validator("confirm_delete")
+ @classmethod
+ def validate_confirm_delete(cls, value: str) -> str:
+ if value != "DELETE":
+ raise ValueError("You must enter 'DELETE' in this field")
+ return value
+
+
+class VotePreviewForm(form.Form):
+ body: str = form.label(description="Body", widget=form.Widget.TEXTAREA)
+ vote_duration: form.Int = form.label(description="Vote duration")
Review Comment:
Should not use `description=`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
Re: [PR] Make the draft forms more type safe (tooling-trusted-releases)
Posted by "andrewmusselman (via GitHub)" <gi...@apache.org>.
andrewmusselman commented on PR #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336#issuecomment-3618301065
I didn't touch those, will fix that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
Re: [PR] Make the draft forms more type safe (tooling-trusted-releases)
Posted by "andrewmusselman (via GitHub)" <gi...@apache.org>.
andrewmusselman commented on PR #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336#issuecomment-3603825294
> There's a problem with deleting a file: the HTML modal dialogue comes up with a new line of text saying `hidden_tag`, and submitting the form in the modal dialogue does not delete the file or even close the dialogue.
>
> The HTML modal dialogue has always been buggy, but it does currently work on `main` for deleting files. We should take this opportunity to standardise on the new `confirm` mechanism that #309 introduces. Once #309 is squashed to a single commit we'll merge it, and then this PR can be updated to use it. Please also remove the entire HTML modal dialogue mechanism, as long as it's not used anywhere else.
Got it, will do
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
[GH] Make the draft forms more type safe (tooling-trusted-releases)
Posted by "sbp (via GitHub)" <gi...@apache.org>.
sbp commented on code in PR #336:
URL: https://github.com/apache/tooling-trusted-releases/pull/336#discussion_r2549913592
##########
atr/templates/check-selected.html:
##########
@@ -213,5 +184,21 @@ <h3 id="delete-draft" class="mt-4">Delete this draft</h3>
{{ super() }}
<script>
init();
+
+ // Delete form validation
Review Comment:
Just a reminder to change to use server side validation, as we discussed, then request a review. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org