Skip to content

Addresses issue #1283. #1286

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

Merged
merged 3 commits into from
Mar 19, 2025
Merged

Conversation

Nigel-Ecma
Copy link
Contributor

Updates §6.2.5 as per issue and also changes the descriptive style to the proscriptive requirement of the Standard.

Issuing as draft to allow time for comments on #1283.

Note: In #1283 I promised test cases for type_argument_list parsing. These have been incorporated into a grammar testing update, #1284, and can be found in tools/GrammarTesting/Tests/Parsing/Samples/v8 samples Type argument list and TAL Query expressions.

Updates §6.2.5 as per issue and also changes the descriptive style to the proscriptive requirement of the Standard
@Nigel-Ecma
Copy link
Contributor Author

@jskeet – I thought I’d fixed the bad references but I missed a § in the one that is still causing the fail, simple fix. However I’ve left it with the error as the Word converter is throwing an exception you might wish to look into. If you drop me a line when you’d like me to remove the stray § I'll push it out.

@jskeet
Copy link
Contributor

jskeet commented Mar 10, 2025

Thanks Nigel - will try to get to that in the next few days.

@jskeet
Copy link
Contributor

jskeet commented Mar 11, 2025

@Nigel-Ecma: Having looked at the error, that feels like the Word Converter "core" itself "working as intended" - it's reporting the error nicely. The error is coming from Utilities.StatusCheckLogger.BuildCheckRunResult which is something @BillWagner knows rather more about than me.

Bill: the job that contains the failure is https://github.com/dotnet/csharpstandard/actions/runs/13755098000/job/38461209510

(I personally don't mind a slightly messy error, so long as it really does fail, and the underlying failure is clear - but I expect it could be tidier.)

@Nigel-Ecma: Having captured that information (which Bill can choose to address or not) I think you're fine to remove the stray §.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

I like all the changes here (although as noted we might want to make the whitespace changes separately, and across the board - and then possibly lint for them).

I think @gafter's input would be useful to validate the change as well. What looks reasonable to me can still be entirely incorrect...

Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

This LGTM once we address open comments (async or in the meeting)

@BillWagner
Copy link
Member

@Nigel-Ecma: Having looked at the error, that feels like the Word Converter "core" itself "working as intended" - it's reporting the error nicely. The error is coming from Utilities.StatusCheckLogger.BuildCheckRunResult which is something @BillWagner knows rather more about than me.

Bill: the job that contains the failure is https://github.com/dotnet/csharpstandard/actions/runs/13755098000/job/38461209510

(I personally don't mind a slightly messy error, so long as it really does fail, and the underlying failure is clear - but I expect it could be tidier.)

The exception from building the check result is what I'm trying to address in #1269. Quick background: When our tools run on a PR, they run in the context of the PR branch, which is often in a fork. That means the tools don't have write access to the base repo. That's a better security practice.

I've asked for some help to get that working, but no answer yet. If I can't get unstuck, I'll remove the extra logging. We'll still get the output, but it won't be integrated into the files tab.

@Nigel-Ecma Nigel-Ecma added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Mar 17, 2025
@Nigel-Ecma Nigel-Ecma marked this pull request as ready for review March 17, 2025 21:18
@jskeet
Copy link
Contributor

jskeet commented Mar 19, 2025

After discussion and raising #1294, we are happy.

@jskeet jskeet merged commit 36a6983 into dotnet:draft-v8 Mar 19, 2025
6 checks passed
@Nigel-Ecma Nigel-Ecma deleted the disambiguation_for_1283 branch March 19, 2025 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants