sbp commented on code in PR #309:
URL:
https://github.com/apache/tooling-trusted-releases/pull/309#discussion_r2557574053
##########
atr/get/distribution.py:
##########
@@ -94,27 +86,80 @@ async def list_get(session: web.Committer, project: str,
version: str) -> str:
shared.distribution.html_tr_a("Web URL", dist.web_url),
]
block.table(".table.table-striped.table-bordered")[tbody]
- form_action = util.as_url(post.distribution.delete, project=project,
version=version)
- delete_form_element = forms.render_simple(
- delete_form,
- action=form_action,
- submit_classes="btn-danger",
- )
- block.append(htm.div(".mb-3")[delete_form_element])
+
+ # TODO: add a confirm: bool keyword argument to form.render, allowing
us to
Review Comment:
This should be fairly trivial, so perhaps it would be a better idea to
implement that within this PR? Otherwise there's a big block of code that we're
adding here which will have to be entirely changed.
##########
atr/form.py:
##########
@@ -640,6 +640,10 @@ def _get_widget_type(field_info:
pydantic.fields.FieldInfo) -> Widget: # noqa:
if annotation in (int, float):
return Widget.NUMBER
+ # Check for plain enum types (including wrapper enums)
Review Comment:
We already handle the `form.Enum[E]` case. What does this code allow us to
do that is not covered by the existing `form.Enum[E]` code? What sort of usage
does it enable?
##########
atr/get/distribution.py:
##########
@@ -94,27 +86,80 @@ async def list_get(session: web.Committer, project: str,
version: str) -> str:
shared.distribution.html_tr_a("Web URL", dist.web_url),
]
block.table(".table.table-striped.table-bordered")[tbody]
- form_action = util.as_url(post.distribution.delete, project=project,
version=version)
- delete_form_element = forms.render_simple(
- delete_form,
- action=form_action,
- submit_classes="btn-danger",
- )
- block.append(htm.div(".mb-3")[delete_form_element])
+
+ # TODO: add a confirm: bool keyword argument to form.render, allowing
us to
+ # use form.render and not have to special case forms with
confirmations.
+ # Create inline delete form with confirmation dialog (following
projects.py pattern)
+ delete_form = htm.form(
+ ".d-inline-block.m-0",
+ method="post",
+ action=util.as_url(post.distribution.delete, project=project,
version=version),
+ onsubmit=(
+ f"return confirm('Are you sure you want to delete the
distribution "
+ f"{dist.platform.name} {dist.package} {dist.version}? This
cannot be undone.');"
+ ),
+ )[
+ form.csrf_input(),
+ htpy.input(type="hidden", name="release_name",
value=dist.release_name),
+ htpy.input(type="hidden", name="platform",
value=dist.platform.name),
+ htpy.input(type="hidden", name="owner_namespace",
value=dist.owner_namespace or ""),
+ htpy.input(type="hidden", name="package", value=dist.package),
+ htpy.input(type="hidden", name="version", value=dist.version),
+ htpy.button(".btn.btn-danger.btn-sm", type="submit",
title=f"Delete {dist.title}")[
+ htpy.i(".bi.bi-trash"), " Delete"
+ ],
+ ]
+ block.append(htm.div(".mb-3")[delete_form])
title = f"Distribution list for {project} {version}"
return await template.blank(title, content=block.collect())
+async def _record_form_page(project: str, version: str, staging: bool) -> str:
Review Comment:
This private function, starting with a single underscore, comes before
public functions. Private functions should always be ordered below public
functions, though I have often wondered if we need to revisit this decision.
##########
atr/post/distribution.py:
##########
@@ -28,17 +28,68 @@
import atr.web as web
+async def record_form_process_page(
+ session: web.Committer,
+ form_data: shared.distribution.DistributeForm,
+ project: str,
+ version: str,
+ /,
+ staging: bool = False,
+) -> web.WerkzeugResponse:
+ platform_wrapper = cast("shared.distribution.DistributionPlatformWrapper",
form_data.platform)
+ sql_platform = shared.distribution.wrapper_to_sql(platform_wrapper)
+ dd = distribution.Data(
+ platform=sql_platform,
+ owner_namespace=form_data.owner_namespace,
+ package=form_data.package,
+ version=form_data.version,
+ details=form_data.details,
+ )
+ release, committee = await
shared.distribution.release_validated_and_committee(
+ project,
+ version,
+ staging=staging,
+ )
+
+ async with
storage.write_as_committee_member(committee_name=committee.name) as w:
+ try:
+ _dist, added, _metadata = await w.distributions.record_from_data(
+ release=release,
+ staging=staging,
+ dd=dd,
+ )
+ except storage.AccessError as e:
+ # Instead of calling record_form_page_new, redirect with error
message
+ return await session.redirect(
+ get.distribution.stage if staging else get.distribution.record,
+ project=project,
+ version=version,
+ error=str(e), # Flash error message
Review Comment:
Per code conventions, we do not use line-ending comments.
##########
atr/shared/distribution.py:
##########
@@ -17,74 +17,125 @@
from __future__ import annotations
-import dataclasses
-import json
-from typing import Literal
+import enum
+from typing import Any, Literal, cast
-import quart
+import pydantic
import atr.db as db
-import atr.forms as forms
+import atr.form as form
import atr.get as get
import atr.htm as htm
import atr.models.distribution as distribution
import atr.models.sql as sql
-import atr.storage as storage
-import atr.template as template
import atr.util as util
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
-
-
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
+class DistributionPlatformWrapper(enum.Enum):
+ """Wrapper enum for distribution platforms."""
+
+ ARTIFACT_HUB = "Artifact Hub"
+ DOCKER_HUB = "Docker Hub"
+ MAVEN = "Maven Central"
+ NPM = "npm"
+ NPM_SCOPED = "npm (scoped)"
+ PYPI = "PyPI"
+
+
+def wrapper_to_sql(wrapper: DistributionPlatformWrapper) ->
sql.DistributionPlatform:
+ """Convert wrapper enum to SQL enum."""
+ match wrapper:
+ case DistributionPlatformWrapper.ARTIFACT_HUB:
+ return sql.DistributionPlatform.ARTIFACT_HUB
+ case DistributionPlatformWrapper.DOCKER_HUB:
+ return sql.DistributionPlatform.DOCKER_HUB
+ case DistributionPlatformWrapper.MAVEN:
+ return sql.DistributionPlatform.MAVEN
+ case DistributionPlatformWrapper.NPM:
+ return sql.DistributionPlatform.NPM
+ case DistributionPlatformWrapper.NPM_SCOPED:
+ return sql.DistributionPlatform.NPM_SCOPED
+ case DistributionPlatformWrapper.PYPI:
+ return sql.DistributionPlatform.PYPI
+
+
+def sql_to_wrapper(platform: sql.DistributionPlatform) ->
DistributionPlatformWrapper:
+ """Convert SQL enum to wrapper enum."""
+ match platform:
+ case sql.DistributionPlatform.ARTIFACT_HUB:
+ return DistributionPlatformWrapper.ARTIFACT_HUB
+ case sql.DistributionPlatform.DOCKER_HUB:
+ return DistributionPlatformWrapper.DOCKER_HUB
+ case sql.DistributionPlatform.MAVEN:
+ return DistributionPlatformWrapper.MAVEN
+ case sql.DistributionPlatform.NPM:
+ return DistributionPlatformWrapper.NPM
+ case sql.DistributionPlatform.NPM_SCOPED:
+ return DistributionPlatformWrapper.NPM_SCOPED
+ case sql.DistributionPlatform.PYPI:
+ return DistributionPlatformWrapper.PYPI
+
+
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
+
+
+class DistributeForm(form.Form):
+ platform: form.Enum[DistributionPlatformWrapper] =
form.label(description="Platform")
+ owner_namespace: str = form.label(
"Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+ """Who owns or names the package (Maven groupId, npm @scope, Docker
namespace,
+ GitHub owner, ArtifactHub repo). Leave blank if not used.""",
)
- package = forms.string("Package", placeholder="E.g. artifactId or
package-name")
- version = forms.string("Version", placeholder="E.g. 1.2.3, without a
leading v")
- details = forms.checkbox("Include details", description="Include the
details of the distribution in the response")
- submit = forms.submit("Record distribution")
-
- async def validate(self, extra_validators: dict | None = None) -> bool:
- if not await super().validate(extra_validators):
- return False
- if not self.platform.data:
- return False
- default_owner_namespace =
self.platform.data.value.default_owner_namespace
- requires_owner_namespace =
self.platform.data.value.requires_owner_namespace
- owner_namespace = self.owner_namespace.data
- # TODO: We should disable the owner_namespace field if it's not
required
- # But that would be a lot of complexity
- # And this validation, which we need to keep, is complex enough
- if default_owner_namespace and (not owner_namespace):
- self.owner_namespace.data = default_owner_namespace
- if requires_owner_namespace and (not owner_namespace):
- msg = f'Platform "{self.platform.data.name}" requires an owner or
namespace.'
- return forms.error(self.owner_namespace, msg)
- if (not requires_owner_namespace) and (not default_owner_namespace)
and owner_namespace:
- msg = f'Platform "{self.platform.data.name}" does not require an
owner or namespace.'
- return forms.error(self.owner_namespace, msg)
- return True
-
-
[email protected]
-class FormProjectVersion:
- form: DistributeForm
- project: str
- version: str
+ package: str = form.label("Package", widget=form.Widget.TEXT)
Review Comment:
We don't use `Widget.TEXT` explicitly with `str`. It's used automatically.
##########
atr/get/distribution.py:
##########
@@ -94,27 +86,80 @@ async def list_get(session: web.Committer, project: str,
version: str) -> str:
shared.distribution.html_tr_a("Web URL", dist.web_url),
]
block.table(".table.table-striped.table-bordered")[tbody]
- form_action = util.as_url(post.distribution.delete, project=project,
version=version)
- delete_form_element = forms.render_simple(
- delete_form,
- action=form_action,
- submit_classes="btn-danger",
- )
- block.append(htm.div(".mb-3")[delete_form_element])
+
+ # TODO: add a confirm: bool keyword argument to form.render, allowing
us to
+ # use form.render and not have to special case forms with
confirmations.
+ # Create inline delete form with confirmation dialog (following
projects.py pattern)
Review Comment:
The `(following projects.py pattern)` part refers to a change between files,
and we avoid adding comments that refer to changes from previous states. I'll
add a code convention about this.
##########
atr/post/distribution.py:
##########
@@ -28,17 +28,68 @@
import atr.web as web
+async def record_form_process_page(
+ session: web.Committer,
+ form_data: shared.distribution.DistributeForm,
+ project: str,
+ version: str,
+ /,
+ staging: bool = False,
+) -> web.WerkzeugResponse:
+ platform_wrapper = cast("shared.distribution.DistributionPlatformWrapper",
form_data.platform)
Review Comment:
I didn't have to use a `cast` in the `ignores.py` modules. I wonder why it's
different here? Could you compare with my code and see what's different? (It's
not immediately obvious to me.)
##########
atr/shared/distribution.py:
##########
@@ -17,74 +17,125 @@
from __future__ import annotations
-import dataclasses
-import json
-from typing import Literal
+import enum
+from typing import Any, Literal, cast
-import quart
+import pydantic
import atr.db as db
-import atr.forms as forms
+import atr.form as form
import atr.get as get
import atr.htm as htm
import atr.models.distribution as distribution
import atr.models.sql as sql
-import atr.storage as storage
-import atr.template as template
import atr.util as util
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
-
-
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
+class DistributionPlatformWrapper(enum.Enum):
+ """Wrapper enum for distribution platforms."""
+
+ ARTIFACT_HUB = "Artifact Hub"
+ DOCKER_HUB = "Docker Hub"
+ MAVEN = "Maven Central"
+ NPM = "npm"
+ NPM_SCOPED = "npm (scoped)"
+ PYPI = "PyPI"
+
+
+def wrapper_to_sql(wrapper: DistributionPlatformWrapper) ->
sql.DistributionPlatform:
+ """Convert wrapper enum to SQL enum."""
+ match wrapper:
+ case DistributionPlatformWrapper.ARTIFACT_HUB:
+ return sql.DistributionPlatform.ARTIFACT_HUB
+ case DistributionPlatformWrapper.DOCKER_HUB:
+ return sql.DistributionPlatform.DOCKER_HUB
+ case DistributionPlatformWrapper.MAVEN:
+ return sql.DistributionPlatform.MAVEN
+ case DistributionPlatformWrapper.NPM:
+ return sql.DistributionPlatform.NPM
+ case DistributionPlatformWrapper.NPM_SCOPED:
+ return sql.DistributionPlatform.NPM_SCOPED
+ case DistributionPlatformWrapper.PYPI:
+ return sql.DistributionPlatform.PYPI
+
+
+def sql_to_wrapper(platform: sql.DistributionPlatform) ->
DistributionPlatformWrapper:
+ """Convert SQL enum to wrapper enum."""
+ match platform:
+ case sql.DistributionPlatform.ARTIFACT_HUB:
+ return DistributionPlatformWrapper.ARTIFACT_HUB
+ case sql.DistributionPlatform.DOCKER_HUB:
+ return DistributionPlatformWrapper.DOCKER_HUB
+ case sql.DistributionPlatform.MAVEN:
+ return DistributionPlatformWrapper.MAVEN
+ case sql.DistributionPlatform.NPM:
+ return DistributionPlatformWrapper.NPM
+ case sql.DistributionPlatform.NPM_SCOPED:
+ return DistributionPlatformWrapper.NPM_SCOPED
+ case sql.DistributionPlatform.PYPI:
+ return DistributionPlatformWrapper.PYPI
+
+
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
+
+
+class DistributeForm(form.Form):
+ platform: form.Enum[DistributionPlatformWrapper] =
form.label(description="Platform")
+ owner_namespace: str = form.label(
"Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+ """Who owns or names the package (Maven groupId, npm @scope, Docker
namespace,
+ GitHub owner, ArtifactHub repo). Leave blank if not used.""",
)
- package = forms.string("Package", placeholder="E.g. artifactId or
package-name")
- version = forms.string("Version", placeholder="E.g. 1.2.3, without a
leading v")
- details = forms.checkbox("Include details", description="Include the
details of the distribution in the response")
- submit = forms.submit("Record distribution")
-
- async def validate(self, extra_validators: dict | None = None) -> bool:
- if not await super().validate(extra_validators):
- return False
- if not self.platform.data:
- return False
- default_owner_namespace =
self.platform.data.value.default_owner_namespace
- requires_owner_namespace =
self.platform.data.value.requires_owner_namespace
- owner_namespace = self.owner_namespace.data
- # TODO: We should disable the owner_namespace field if it's not
required
- # But that would be a lot of complexity
- # And this validation, which we need to keep, is complex enough
- if default_owner_namespace and (not owner_namespace):
- self.owner_namespace.data = default_owner_namespace
- if requires_owner_namespace and (not owner_namespace):
- msg = f'Platform "{self.platform.data.name}" requires an owner or
namespace.'
- return forms.error(self.owner_namespace, msg)
- if (not requires_owner_namespace) and (not default_owner_namespace)
and owner_namespace:
- msg = f'Platform "{self.platform.data.name}" does not require an
owner or namespace.'
- return forms.error(self.owner_namespace, msg)
- return True
-
-
[email protected]
-class FormProjectVersion:
- form: DistributeForm
- project: str
- version: str
+ package: str = form.label("Package", widget=form.Widget.TEXT)
+ version: str = form.label("Version", widget=form.Widget.TEXT)
+ details: form.Bool = form.label(
+ "Include details",
+ "Include the details of the distribution in the response",
+ )
+
+ @pydantic.field_validator("platform", mode="before")
+ @classmethod
+ def validate_platform(cls, value: Any) -> DistributionPlatformWrapper:
+ if isinstance(value, DistributionPlatformWrapper):
Review Comment:
Which type does this method actually receive in practice?
##########
atr/shared/distribution.py:
##########
@@ -17,74 +17,125 @@
from __future__ import annotations
-import dataclasses
-import json
-from typing import Literal
+import enum
+from typing import Any, Literal, cast
-import quart
+import pydantic
import atr.db as db
-import atr.forms as forms
+import atr.form as form
import atr.get as get
import atr.htm as htm
import atr.models.distribution as distribution
import atr.models.sql as sql
-import atr.storage as storage
-import atr.template as template
import atr.util as util
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
-
-
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
+class DistributionPlatformWrapper(enum.Enum):
+ """Wrapper enum for distribution platforms."""
+
+ ARTIFACT_HUB = "Artifact Hub"
+ DOCKER_HUB = "Docker Hub"
+ MAVEN = "Maven Central"
+ NPM = "npm"
+ NPM_SCOPED = "npm (scoped)"
+ PYPI = "PyPI"
+
+
+def wrapper_to_sql(wrapper: DistributionPlatformWrapper) ->
sql.DistributionPlatform:
+ """Convert wrapper enum to SQL enum."""
+ match wrapper:
+ case DistributionPlatformWrapper.ARTIFACT_HUB:
+ return sql.DistributionPlatform.ARTIFACT_HUB
+ case DistributionPlatformWrapper.DOCKER_HUB:
+ return sql.DistributionPlatform.DOCKER_HUB
+ case DistributionPlatformWrapper.MAVEN:
+ return sql.DistributionPlatform.MAVEN
+ case DistributionPlatformWrapper.NPM:
+ return sql.DistributionPlatform.NPM
+ case DistributionPlatformWrapper.NPM_SCOPED:
+ return sql.DistributionPlatform.NPM_SCOPED
+ case DistributionPlatformWrapper.PYPI:
+ return sql.DistributionPlatform.PYPI
+
+
+def sql_to_wrapper(platform: sql.DistributionPlatform) ->
DistributionPlatformWrapper:
+ """Convert SQL enum to wrapper enum."""
+ match platform:
+ case sql.DistributionPlatform.ARTIFACT_HUB:
+ return DistributionPlatformWrapper.ARTIFACT_HUB
+ case sql.DistributionPlatform.DOCKER_HUB:
+ return DistributionPlatformWrapper.DOCKER_HUB
+ case sql.DistributionPlatform.MAVEN:
+ return DistributionPlatformWrapper.MAVEN
+ case sql.DistributionPlatform.NPM:
+ return DistributionPlatformWrapper.NPM
+ case sql.DistributionPlatform.NPM_SCOPED:
+ return DistributionPlatformWrapper.NPM_SCOPED
+ case sql.DistributionPlatform.PYPI:
+ return DistributionPlatformWrapper.PYPI
+
+
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
+
+
+class DistributeForm(form.Form):
+ platform: form.Enum[DistributionPlatformWrapper] =
form.label(description="Platform")
Review Comment:
We don't use keywords with `form.label`. I guess the wider convention is
that we don't use keywords for arguments before `*`, but I'm not sure how
widely that holds and if it's really a worthwhile convention.
##########
atr/post/distribution.py:
##########
@@ -17,7 +17,7 @@
from __future__ import annotations
-import quart
+from typing import cast
Review Comment:
Per the code conventions, we do not use `cast` or type ignores unless there
is strictly no other alternative.
##########
atr/shared/distribution.py:
##########
@@ -17,74 +17,125 @@
from __future__ import annotations
-import dataclasses
-import json
-from typing import Literal
+import enum
+from typing import Any, Literal, cast
-import quart
+import pydantic
import atr.db as db
-import atr.forms as forms
+import atr.form as form
import atr.get as get
import atr.htm as htm
import atr.models.distribution as distribution
import atr.models.sql as sql
-import atr.storage as storage
-import atr.template as template
import atr.util as util
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
-
-
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
+class DistributionPlatformWrapper(enum.Enum):
+ """Wrapper enum for distribution platforms."""
+
+ ARTIFACT_HUB = "Artifact Hub"
+ DOCKER_HUB = "Docker Hub"
+ MAVEN = "Maven Central"
+ NPM = "npm"
+ NPM_SCOPED = "npm (scoped)"
+ PYPI = "PyPI"
+
+
+def wrapper_to_sql(wrapper: DistributionPlatformWrapper) ->
sql.DistributionPlatform:
Review Comment:
When I wrote the original enum mapper functions, I ignored our code
convention to order functions alphabetically in order to keep them with the
enum. But I guess they should be class methods on the enum itself? Can you try
that and see if that approach works? You might need to use strings for the type
annotations.
##########
atr/post/distribution.py:
##########
@@ -28,17 +28,68 @@
import atr.web as web
+async def record_form_process_page(
Review Comment:
This interface is not alphabetically ordered.
##########
atr/post/distribution.py:
##########
@@ -28,17 +28,68 @@
import atr.web as web
+async def record_form_process_page(
+ session: web.Committer,
+ form_data: shared.distribution.DistributeForm,
+ project: str,
+ version: str,
+ /,
+ staging: bool = False,
+) -> web.WerkzeugResponse:
+ platform_wrapper = cast("shared.distribution.DistributionPlatformWrapper",
form_data.platform)
+ sql_platform = shared.distribution.wrapper_to_sql(platform_wrapper)
+ dd = distribution.Data(
+ platform=sql_platform,
+ owner_namespace=form_data.owner_namespace,
+ package=form_data.package,
+ version=form_data.version,
+ details=form_data.details,
+ )
+ release, committee = await
shared.distribution.release_validated_and_committee(
+ project,
+ version,
+ staging=staging,
+ )
+
+ async with
storage.write_as_committee_member(committee_name=committee.name) as w:
+ try:
+ _dist, added, _metadata = await w.distributions.record_from_data(
+ release=release,
+ staging=staging,
+ dd=dd,
+ )
+ except storage.AccessError as e:
+ # Instead of calling record_form_page_new, redirect with error
message
+ return await session.redirect(
+ get.distribution.stage if staging else get.distribution.record,
+ project=project,
+ version=version,
+ error=str(e), # Flash error message
+ )
+
+ # Success - redirect to distribution list with success message
+ message = "Distribution recorded successfully." if added else
"Distribution was already recorded."
+ return await session.redirect(
+ get.distribution.list_get,
+ project=project,
+ version=version,
+ success=message,
+ )
+
+
@post.committer("/distribution/delete/<project>/<version>")
-async def delete(session: web.Committer, project: str, version: str) ->
web.WerkzeugResponse:
- form = await shared.distribution.DeleteForm.create_form(data=await
quart.request.form)
- dd = distribution.DeleteData.model_validate(form.data)
[email protected](shared.distribution.DeleteForm)
+async def delete(
+ session: web.Committer, delete_form: shared.distribution.DeleteForm,
project: str, version: str
+) -> web.WerkzeugResponse:
+ dd = distribution.DeleteData.model_construct(**delete_form.model_dump())
Review Comment:
As I recall, `DeleteData` was one of the original motivations for adding the
`form.py` module. It was one of the first models that I added to receive data
from a WTForms form, to make it more type safe. Now that we have `form.py`, we
could potentially use `DeleteForm` directly. The main difference is `platform:
str = form.label("Platform", widget=form.Widget.HIDDEN)`, which is not an enum,
but note how we handled that on the original `DeleteData` class:
```python
@pydantic.field_validator("platform", mode="before")
@classmethod
def coerce_platform(cls, v: object) -> object:
if isinstance(v, str):
return sql.DistributionPlatform[v]
return v
```
So we can presumably continue to do that, though this might require
something like interaction between `form.Enum[E]` and `Widget.HIDDEN`. Can you
see if you can make this work, please? If you run into trouble, we can discuss
it.
##########
atr/shared/distribution.py:
##########
@@ -17,74 +17,125 @@
from __future__ import annotations
-import dataclasses
-import json
-from typing import Literal
+import enum
+from typing import Any, Literal, cast
-import quart
+import pydantic
import atr.db as db
-import atr.forms as forms
+import atr.form as form
import atr.get as get
import atr.htm as htm
import atr.models.distribution as distribution
import atr.models.sql as sql
-import atr.storage as storage
-import atr.template as template
import atr.util as util
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
-
-
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
+class DistributionPlatformWrapper(enum.Enum):
+ """Wrapper enum for distribution platforms."""
+
+ ARTIFACT_HUB = "Artifact Hub"
+ DOCKER_HUB = "Docker Hub"
+ MAVEN = "Maven Central"
+ NPM = "npm"
+ NPM_SCOPED = "npm (scoped)"
+ PYPI = "PyPI"
+
+
+def wrapper_to_sql(wrapper: DistributionPlatformWrapper) ->
sql.DistributionPlatform:
+ """Convert wrapper enum to SQL enum."""
+ match wrapper:
+ case DistributionPlatformWrapper.ARTIFACT_HUB:
+ return sql.DistributionPlatform.ARTIFACT_HUB
+ case DistributionPlatformWrapper.DOCKER_HUB:
+ return sql.DistributionPlatform.DOCKER_HUB
+ case DistributionPlatformWrapper.MAVEN:
+ return sql.DistributionPlatform.MAVEN
+ case DistributionPlatformWrapper.NPM:
+ return sql.DistributionPlatform.NPM
+ case DistributionPlatformWrapper.NPM_SCOPED:
+ return sql.DistributionPlatform.NPM_SCOPED
+ case DistributionPlatformWrapper.PYPI:
+ return sql.DistributionPlatform.PYPI
+
+
+def sql_to_wrapper(platform: sql.DistributionPlatform) ->
DistributionPlatformWrapper:
+ """Convert SQL enum to wrapper enum."""
+ match platform:
+ case sql.DistributionPlatform.ARTIFACT_HUB:
+ return DistributionPlatformWrapper.ARTIFACT_HUB
+ case sql.DistributionPlatform.DOCKER_HUB:
+ return DistributionPlatformWrapper.DOCKER_HUB
+ case sql.DistributionPlatform.MAVEN:
+ return DistributionPlatformWrapper.MAVEN
+ case sql.DistributionPlatform.NPM:
+ return DistributionPlatformWrapper.NPM
+ case sql.DistributionPlatform.NPM_SCOPED:
+ return DistributionPlatformWrapper.NPM_SCOPED
+ case sql.DistributionPlatform.PYPI:
+ return DistributionPlatformWrapper.PYPI
+
+
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
+
+
+class DistributeForm(form.Form):
+ platform: form.Enum[DistributionPlatformWrapper] =
form.label(description="Platform")
+ owner_namespace: str = form.label(
"Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+ """Who owns or names the package (Maven groupId, npm @scope, Docker
namespace,
Review Comment:
The original string syntax avoids the `"\n "` in the middle.
##########
atr/shared/distribution.py:
##########
@@ -17,74 +17,125 @@
from __future__ import annotations
-import dataclasses
-import json
-from typing import Literal
+import enum
+from typing import Any, Literal, cast
-import quart
+import pydantic
import atr.db as db
-import atr.forms as forms
+import atr.form as form
import atr.get as get
import atr.htm as htm
import atr.models.distribution as distribution
import atr.models.sql as sql
-import atr.storage as storage
-import atr.template as template
import atr.util as util
type Phase = Literal["COMPOSE", "VOTE", "FINISH"]
-class DeleteForm(forms.Typed):
- release_name = forms.hidden()
- platform = forms.hidden()
- owner_namespace = forms.hidden()
- package = forms.hidden()
- version = forms.hidden()
- submit = forms.submit("Delete")
-
-
-class DistributeForm(forms.Typed):
- platform = forms.select("Platform", choices=sql.DistributionPlatform)
- owner_namespace = forms.optional(
+class DistributionPlatformWrapper(enum.Enum):
+ """Wrapper enum for distribution platforms."""
+
+ ARTIFACT_HUB = "Artifact Hub"
+ DOCKER_HUB = "Docker Hub"
+ MAVEN = "Maven Central"
+ NPM = "npm"
+ NPM_SCOPED = "npm (scoped)"
+ PYPI = "PyPI"
+
+
+def wrapper_to_sql(wrapper: DistributionPlatformWrapper) ->
sql.DistributionPlatform:
+ """Convert wrapper enum to SQL enum."""
+ match wrapper:
+ case DistributionPlatformWrapper.ARTIFACT_HUB:
+ return sql.DistributionPlatform.ARTIFACT_HUB
+ case DistributionPlatformWrapper.DOCKER_HUB:
+ return sql.DistributionPlatform.DOCKER_HUB
+ case DistributionPlatformWrapper.MAVEN:
+ return sql.DistributionPlatform.MAVEN
+ case DistributionPlatformWrapper.NPM:
+ return sql.DistributionPlatform.NPM
+ case DistributionPlatformWrapper.NPM_SCOPED:
+ return sql.DistributionPlatform.NPM_SCOPED
+ case DistributionPlatformWrapper.PYPI:
+ return sql.DistributionPlatform.PYPI
+
+
+def sql_to_wrapper(platform: sql.DistributionPlatform) ->
DistributionPlatformWrapper:
+ """Convert SQL enum to wrapper enum."""
+ match platform:
+ case sql.DistributionPlatform.ARTIFACT_HUB:
+ return DistributionPlatformWrapper.ARTIFACT_HUB
+ case sql.DistributionPlatform.DOCKER_HUB:
+ return DistributionPlatformWrapper.DOCKER_HUB
+ case sql.DistributionPlatform.MAVEN:
+ return DistributionPlatformWrapper.MAVEN
+ case sql.DistributionPlatform.NPM:
+ return DistributionPlatformWrapper.NPM
+ case sql.DistributionPlatform.NPM_SCOPED:
+ return DistributionPlatformWrapper.NPM_SCOPED
+ case sql.DistributionPlatform.PYPI:
+ return DistributionPlatformWrapper.PYPI
+
+
+class DeleteForm(form.Form):
+ release_name: str = form.label("Release name", widget=form.Widget.HIDDEN)
+ platform: str = form.label("Platform", widget=form.Widget.HIDDEN)
+ owner_namespace: str = form.label("Owner namespace",
widget=form.Widget.HIDDEN)
+ package: str = form.label("Package", widget=form.Widget.HIDDEN)
+ version: str = form.label("Version", widget=form.Widget.HIDDEN)
+
+
+class DistributeForm(form.Form):
+ platform: form.Enum[DistributionPlatformWrapper] =
form.label(description="Platform")
+ owner_namespace: str = form.label(
"Owner or Namespace",
- placeholder="E.g. com.example or scope or library",
- description="Who owns or names the package (Maven groupId, npm @scope,
"
- "Docker namespace, GitHub owner, ArtifactHub repo). Leave blank if not
used.",
+ """Who owns or names the package (Maven groupId, npm @scope, Docker
namespace,
+ GitHub owner, ArtifactHub repo). Leave blank if not used.""",
)
- package = forms.string("Package", placeholder="E.g. artifactId or
package-name")
- version = forms.string("Version", placeholder="E.g. 1.2.3, without a
leading v")
- details = forms.checkbox("Include details", description="Include the
details of the distribution in the response")
- submit = forms.submit("Record distribution")
-
- async def validate(self, extra_validators: dict | None = None) -> bool:
- if not await super().validate(extra_validators):
- return False
- if not self.platform.data:
- return False
- default_owner_namespace =
self.platform.data.value.default_owner_namespace
- requires_owner_namespace =
self.platform.data.value.requires_owner_namespace
- owner_namespace = self.owner_namespace.data
- # TODO: We should disable the owner_namespace field if it's not
required
- # But that would be a lot of complexity
- # And this validation, which we need to keep, is complex enough
- if default_owner_namespace and (not owner_namespace):
- self.owner_namespace.data = default_owner_namespace
- if requires_owner_namespace and (not owner_namespace):
- msg = f'Platform "{self.platform.data.name}" requires an owner or
namespace.'
- return forms.error(self.owner_namespace, msg)
- if (not requires_owner_namespace) and (not default_owner_namespace)
and owner_namespace:
- msg = f'Platform "{self.platform.data.name}" does not require an
owner or namespace.'
- return forms.error(self.owner_namespace, msg)
- return True
-
-
[email protected]
-class FormProjectVersion:
- form: DistributeForm
- project: str
- version: str
+ package: str = form.label("Package", widget=form.Widget.TEXT)
+ version: str = form.label("Version", widget=form.Widget.TEXT)
+ details: form.Bool = form.label(
+ "Include details",
+ "Include the details of the distribution in the response",
+ )
+
+ @pydantic.field_validator("platform", mode="before")
+ @classmethod
+ def validate_platform(cls, value: Any) -> DistributionPlatformWrapper:
+ if isinstance(value, DistributionPlatformWrapper):
+ return value
+ elif isinstance(value, str):
+ for member in DistributionPlatformWrapper:
+ if member.value == value:
+ return member
+ raise ValueError(f"Invalid platform: {value}")
+ raise ValueError(f"Platform must be a string or DistributionPlatform,
got {type(value)}")
+
+ @pydantic.model_validator(mode="after")
+ def validate_owner_namespace(self) -> DistributeForm:
+ if not self.platform:
+ raise ValueError("Platform is required")
+
+ # Help the type checker understand the actual runtime type
+ platform = cast("DistributionPlatformWrapper", self.platform)
Review Comment:
We don't use `cast`, as mentioned in a prior review comment.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
