Skip to content

Conversation

jleightcap
Copy link
Contributor

@jleightcap jleightcap commented May 18, 2023

WIP

Resolves #13695

@jleightcap
Copy link
Contributor Author

Naive global find/replace fails a bunch of tests. Re-running test suite with actual verbose output to see how much the failures share in common.

@jleightcap
Copy link
Contributor Author

jleightcap commented May 18, 2023

For test_validate_token_scope_invalid_project, the existing test fails because of where the error string is derived from:

    def test_validate_token_scope_invalid_project(self):
        form = forms.CreateMacaroonForm(
            data={"description": "dummy", "token_scope": "scope:project:foo"},
            user_id=pretend.stub(),
            macaroon_service=pretend.stub(get_macaroon_by_description=lambda *a: None),
            project_names=["bar"],
        )

        assert not form.validate()
>       assert form.token_scope.errors.pop() == "Unknown or invalid project name: foo"
E       AssertionError: assert 'Specify the token scope' == 'Unknown or invalid project name: foo'
E         - Unknown or invalid project name: foo
E         + Specify the token scope

This makes sense to me. Going through the test errors to move the error strings to the construction-time failures which are now thrown first.

Signed-off-by: Jack Leightcap <[email protected]>
@jleightcap
Copy link
Contributor Author

The addition of the MultiDict import has raised typing stub errors;

warehouse/views.py:43: error: Skipping analyzing "webob.multidict": module is installed, but missing library stubs or py.typed marker  [import]
warehouse/accounts/views.py:32: error: Skipping analyzing "webob.multidict": module is installed, but missing library stubs or py.typed marker  [import]
warehouse/accounts/views.py:32: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
Found 2 errors in 2 files (checked 336 source files)

Because of the addition webob.multidict in the warehouse/ dir.

@jleightcap
Copy link
Contributor Author

One last error exists, having to do with type coercion of a datetime object no longer being performed by the validator,

 def test_valid_data(self, banner_data):
        form = views.BannerForm(formdata=MultiDict(banner_data))
        assert form.validate() is True
        data = form.data
        defaults = {
            "fa_icon": Banner.DEFAULT_FA_ICON,
            "active": False,
            "link_label": Banner.DEFAULT_BTN_LABEL,
        }
>       assert data == {**banner_data, **defaults}
# ...
E         Differing items:
E         {'end': datetime.date(2023, 5, 27)} != {'end': '2023-05-27'}
E         Full diff:
E           {
E            'active': False,
E         -  'end': '2023-05-27',
E         +  'end': datetime.date(2023, 5, 27),
E            'fa_icon': 'fa-comment-alt',
E            'link_label': 'See more',
E            'link_url': 'https://samplebanner.com',
E            'name': 'Sample Banner',
E            'text': 'This should be the correct text',
E           }

@jleightcap jleightcap marked this pull request as ready for review May 25, 2023 20:44
@jleightcap jleightcap requested a review from a team as a code owner May 25, 2023 20:44
@miketheman
Copy link
Member

The addition of the MultiDict import has raised typing stub errors;
...
Because of the addition webob.multidict in the warehouse/ dir.

Not all packages have type hints. There's an open PR on Typeshed for WebOb - so our convention has been to add an excluded entry to pyproject.toml and drop a link to the PR so we can check on it at a later date.

@woodruffw woodruffw self-assigned this May 25, 2023
There's no current tracking issue for supporting type hints
in WebOb.

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member

@miketheman Guessing you might know this 🙂 -- do you happen to know if there's a way to forbid DataRequired in new PRs going forwards? We probably don't want to accidentally regress on this.

Signed-off-by: William Woodruff <[email protected]>
@miketheman
Copy link
Member

do you happen to know if there's a way to forbid DataRequired in new PRs going forwards? We probably don't want to accidentally regress on this.

We currently use Flake8 for linting, so I could imagine a Flake8 Local Plugin that disallows DataRequired. https://flake8.pycqa.org/en/latest/user/configuration.html#using-local-plugins
I haven't done one of these myself yet, but that looks like it might prove useful.

@woodruffw
Copy link
Member

Looks like https://pypi.org/project/flake8-tidy-imports/ might be a good fit as well for that! I think ruff also has similar support.

I'll punt on both of those for this PR, though 🙂

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member

Nice work @jleightcap!

CC @di this should be ready for review now.

@di di merged commit 95f89b3 into pypi:main May 25, 2023
@woodruffw woodruffw deleted the jl/13695 branch May 25, 2023 22:14
@ewdurbin ewdurbin mentioned this pull request May 25, 2023
di added a commit to di/warehouse that referenced this pull request May 25, 2023
di added a commit that referenced this pull request May 25, 2023
jleightcap added a commit to trail-of-forks/warehouse that referenced this pull request May 26, 2023
…3696)

* DataRequired -> InputRequired

Signed-off-by: Jack Leightcap <[email protected]>

* `make translations`

Signed-off-by: William Woodruff <[email protected]>

* pyproject: exclude webob.* from mypy

There's no current tracking issue for supporting type hints
in WebOb.

Signed-off-by: William Woodruff <[email protected]>

* pyproject: tracking issue

Signed-off-by: William Woodruff <[email protected]>

* tests: mash a type into place

Signed-off-by: William Woodruff <[email protected]>

---------

Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: William Woodruff <[email protected]>
jleightcap added a commit to trail-of-forks/warehouse that referenced this pull request Jun 1, 2023
…3696)

* DataRequired -> InputRequired

Signed-off-by: Jack Leightcap <[email protected]>

* `make translations`

Signed-off-by: William Woodruff <[email protected]>

* pyproject: exclude webob.* from mypy

There's no current tracking issue for supporting type hints
in WebOb.

Signed-off-by: William Woodruff <[email protected]>

* pyproject: tracking issue

Signed-off-by: William Woodruff <[email protected]>

* tests: mash a type into place

Signed-off-by: William Woodruff <[email protected]>

---------

Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: William Woodruff <[email protected]>
di pushed a commit to trail-of-forks/warehouse that referenced this pull request Jun 5, 2023
…3696)

* DataRequired -> InputRequired

Signed-off-by: Jack Leightcap <[email protected]>

* `make translations`

Signed-off-by: William Woodruff <[email protected]>

* pyproject: exclude webob.* from mypy

There's no current tracking issue for supporting type hints
in WebOb.

Signed-off-by: William Woodruff <[email protected]>

* pyproject: tracking issue

Signed-off-by: William Woodruff <[email protected]>

* tests: mash a type into place

Signed-off-by: William Woodruff <[email protected]>

---------

Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: William Woodruff <[email protected]>
di pushed a commit that referenced this pull request Jun 5, 2023
* Prefer `InputRequired` over `DataRequired` on form validation (#13696)

* DataRequired -> InputRequired

Signed-off-by: Jack Leightcap <[email protected]>

* `make translations`

Signed-off-by: William Woodruff <[email protected]>

* pyproject: exclude webob.* from mypy

There's no current tracking issue for supporting type hints
in WebOb.

Signed-off-by: William Woodruff <[email protected]>

* pyproject: tracking issue

Signed-off-by: William Woodruff <[email protected]>

* tests: mash a type into place

Signed-off-by: William Woodruff <[email protected]>

---------

Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: William Woodruff <[email protected]>

* Validate views Forms

Signed-off-by: Jack Leightcap <[email protected]>

Validated CreateMacaroonForm

Signed-off-by: Jack Leightcap <[email protected]>

* forms: manage.views.teams

Signed-off-by: Jack Leightcap <[email protected]>

* forms: Manage

Signed-off-by: Jack Leightcap <[email protected]>

* forms: OIDC

Signed-off-by: Jack Leightcap <[email protected]>

* forms: base

Signed-off-by: Jack Leightcap <[email protected]>

* forms: Accounts

Signed-off-by: Jack Leightcap <[email protected]>

* forms: admin.views.users

Signed-off-by: Jack Leightcap <[email protected]>

* forms: admin.views.sponsors

Signed-off-by: Jack Leightcap <[email protected]>

* forms: admin.views.banners

Signed-off-by: Jack Leightcap <[email protected]>

* forms: manage.views.organizations

Signed-off-by: Jack Leightcap <[email protected]>

* forms: functional changes

Signed-off-by: Jack Leightcap <[email protected]>

* tests, warehouse: fix password reset form

Signed-off-by: William Woodruff <[email protected]>

* locale: `make translations`

Signed-off-by: William Woodruff <[email protected]>

* tests: remove deleted form tests

Signed-off-by: William Woodruff <[email protected]>

---------

Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Co-authored-by: William Woodruff <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chore: Forms: replace DataRequired with InputRequired
4 participants