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

Update diagnostic messages and linter rules #5419

Merged
merged 3 commits into from
Dec 20, 2023
Merged

Conversation

parlough
Copy link
Member

@parlough parlough commented Dec 20, 2023

These changes were previously reviewed on Gerrit before landing in the SDK.

Copy link

github-actions bot commented Dec 20, 2023

Visit the preview URL for this PR (updated for commit a603de5):

https://dart-dev--pr5419-misc-diagnostics-upd-c7kvhae3.web.app

(expires Wed, 27 Dec 2023 17:41:22 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: d851bc446d3c4d7394c5406c6f07255afc7075f3

@parlough parlough marked this pull request as ready for review December 20, 2023 05:02
@MaryaBelanger
Copy link
Contributor

I think these are the diagnostics that are causing issues: dart-lang/sdk#54416

If that's correct, should we wait until that's sorted out to copy them here?

@bwilkerson
Copy link
Member

One of the problems was that the error range wasn't highlighted. That's been fixed.

The remaining problems are compilation errors in the code (things like declaring a non-nullable top-level variable without initializing it). I don't know how likely that is to create problems for users.

I also don't know how long it will take to get them updated given the holiday season.

@MaryaBelanger
Copy link
Contributor

MaryaBelanger commented Dec 20, 2023

Thanks for the context @bwilkerson

I'm ok with landing this then, but I wonder @parlough would it be ok to just manually add the fixes from https://dart-review.googlesource.com/c/sdk/+/342941 ?

It's basically just adding external on NATIVE_FIELD_INVALID_TYPE and NATIVE_FIELD_MISSING_TYPE, lines 14091 and 14102, and 14130 and 14144, respectively. And then adding final to NATIVE_FIELD_NOT_STATIC, line 14209.

Idk if that messes up the process you use for diagnostics but I figured from an information standpoint it's probably safe? If not, again just land it, no problem imo.

@bwilkerson
Copy link
Member

I was pleasantly surprised by how quickly the issue was fixed. The try bots that test the changes haven't completed, so I can't confirm that all of the issues are fixed by those changes, but I suspect they will be.

Applying changes to the website that haven't yet landed in analyzer/messages.yaml is fine as long as those changes will also be applied in that file (which I believe they will).

@parlough
Copy link
Member Author

Simon has always been super helpful and on top of things =D

@parlough parlough merged commit 4270c61 into main Dec 20, 2023
9 checks passed
@parlough parlough deleted the misc/diagnostics-updates branch December 20, 2023 17:48
atsansone pushed a commit to atsansone/site-www that referenced this pull request Jan 26, 2024
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.

3 participants