Skip to content

Conversation

JohaJung
Copy link
Contributor

This basically implements what was proposed by @IwanVosloo in #8. Find inputs outside the form tag itself by searching for input elements in the whole response HTML to their form attribute.

closes #8

This is my first PR in this repository, so if there are any style/workflow guidelines I did not recognize or misunderstood, please let me know and I'll try to improve :)

By searching for input elements also outside the form tag itself, which belong to this form according to their `form` attribute.

closes Pylons#8
@gawel
Copy link
Member

gawel commented Apr 1, 2025

This will require some more tests.

Also you're not keeping field orders. You're just adding html5 fields at the end.

I'm not sure it's a very usefull feature but since it exists it should always works.

@JohaJung
Copy link
Contributor Author

JohaJung commented Apr 1, 2025

I changed the logic to preserve field order.

Also I wrote a basic test ensuring that inputs are found and kept in correct order. What else are properties you'd like to have tested?

@septatrix
Copy link

@gawel Any chance to get an update on this? Your previous code-related comments seem to have been applied.

While I also agree with you that in most cases it is preferable to structure ones html such that the form attribute is not necessary, this is not always possible. We would also like to have this feature available for our codebase :)

webtest/forms.py Outdated
tags = ('input', 'select', 'textarea', 'button')
for pos, node in enumerate(self.html.find_all(tags)):
inner_elts = self.html.find_all(tags)
if self.response:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you need this. Is there any case where response is None ? Is it just for testing ?

Copy link
Contributor Author

@JohaJung JohaJung Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not deep enough into it to confirm response could be none in real applications, but in this repo there is TestFormLint::test_form_lint which initializes a Form with response None (and this test errored on my first attempt without this catch). So I guess it's just for testing.

Copy link
Member

@gawel gawel Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's the only test, can you try to use webtest.TestResponse('<body></body>') instead of None ? Not sure it'll work but it will be cool to avoid this if

Copy link
Contributor Author

@JohaJung JohaJung Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exact proposal did not work, but embedding the form html that is being linted in a <body>-Tag worked.
(Taking only the form html without surrounding body did work, too. But I considered this one line more worth having actual valid HTML in the response)

Thanks for commenting and helping out!

@gawel
Copy link
Member

gawel commented Apr 25, 2025

@gawel Any chance to get an update on this? Your previous code-related comments seem to have been applied.

I just added a comment but yes, seems fair enough

make the response more likely to be a real-world response
@gawel gawel merged commit fae253d into Pylons:main Apr 25, 2025
14 checks passed
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.

HTML5 form support
3 participants