Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't include a file called .htmlvalidate.json by default (OSOE-844) #359

Closed
sarahelsaig opened this issue Apr 29, 2024 · 7 comments · Fixed by #361
Closed

Don't include a file called .htmlvalidate.json by default (OSOE-844) #359

sarahelsaig opened this issue Apr 29, 2024 · 7 comments · Fixed by #361
Assignees
Labels
enhancement New feature or request

Comments

@sarahelsaig
Copy link
Member

sarahelsaig commented Apr 29, 2024

Right now we include .htmlvalidate.json in the project csproj file. This means if you place your own .htmlvalidate.json file in a project that references the NuGet package, it won't be enough. You also have to remove the import from the package first. This is not intuitive and as far as I can see it's not documented either.

Instead, I suggest renaming it to something like default.htmlvalidate.json and including a HtmlValidationOptions out of the box that uses .htmlvalidate.json if the file exists and default.htmlvalidate.json if it's missing.


Also I suggest including "form-dup-name": "off" in the default file, because we we have to turn that rule off for every project that uses boolean fields or just stock ASP.NET Core checkbox tag helpers because it expands into stuff like this:

<input name="OrderPart.IsCorporation.Value" type="hidden" value="false"><input name="OrderPart.BillingAndShippingAddressesMatch.Value" type="hidden" value="false">

Also backport this into UITT.

Jira issue

@github-actions github-actions bot changed the title Don't include a file called .htmlvalidate.json by default Don't include a file called .htmlvalidate.json by default (OSOE-844) Apr 29, 2024
@Piedone Piedone added the enhancement New feature or request label Apr 29, 2024
@Piedone
Copy link
Member

Piedone commented Apr 29, 2024

Is the linked configuration a suitable workaround for you now? I.e., is this only about making configuration more convenient, or do we lack some functionality?

form-dup-name fails on input fields with duplicate names. Why would that fail on the example code? The names there are unique.

@sarahelsaig
Copy link
Member Author

Is the linked configuration a suitable workaround for you now?

Actually no. Now the Linux run is broken that previously worked. 🫠

form-dup-name fails on input fields with duplicate names. Why would that fail on the example code? The names there are unique.

Oof, I pasted without thinking. Anyway, here it is:
image
And here is the responsible code. (this is caused by Microsoft.AspNetCore.Mvc.TagHelpers.InputTagHelper)
I guess the purpose of the duplicate names to send the default false value if the actual checkbox is not checked. Normal HTML behavior is to not send anything in POST when the box is unchecked. I suppose the ASP.NET Core folks wanted to avoid the ambiguity between false and missing.

@Piedone
Copy link
Member

Piedone commented Apr 29, 2024

Please work on this if it's blocking for you.

OK, indeed we can use "form-dup-name": "off" in the default config.

@Piedone
Copy link
Member

Piedone commented Apr 30, 2024

Following up from OrchardCMS/OrchardCore.Commerce#434, also include here the following:

  • Document how to use extends in the json to extend the default file.
  • Also add a per-test timeout configuration, see here.

@Piedone
Copy link
Member

Piedone commented May 2, 2024

Hmm, we can actually set the timeout of async tests in xUnit directly: https://www.reddit.com/r/csharp/comments/zlulro/did_you_know_you_could_set_a_timeout_in_your/ So, I don't think we need a custom config, unless we can do a timeout that excludes waiting for the setup operation (that may be started by another test). However, that might work with the latest xUnit only, what I've updated to under #355.

@sarahelsaig
Copy link
Member Author

Unless I misunderstood this is opt-in, so you'd have to add the Timeout to every Fact individually. Having a required limit that you can configure globally or per-test is still worth it.

@Piedone
Copy link
Member

Piedone commented May 3, 2024

Yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants