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

fix: Make warning 232 pedantic #1986

Merged
merged 6 commits into from
Jul 23, 2021

Conversation

willemneal
Copy link
Contributor

Because of tree shaking unused methods or class aren't compiled so there is no need to warn about unimplemented instances.

Because of tree shaking unused methods or class aren't compiled so there is no need to warn about unimplemented instances.
@willemneal
Copy link
Contributor Author

@dcodeIO Should I remove it from the diagnosticMessages* too?

@dcodeIO
Copy link
Member

dcodeIO commented Jul 15, 2021

I thought we agreed to make it a pedantic message, i.e. move it to the pedantic range + pedantic check?

@willemneal
Copy link
Contributor Author

Oh you're right!

@willemneal
Copy link
Contributor Author

Not sure what you mean pendatic check, it seems the one exported-generic test passes since it doesn't look for WARNING.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 15, 2021

It still emits the pedantic message, though. Needs if (this.options.pedantic) { ... emit the diagnostic ... }.

@willemneal willemneal requested a review from dcodeIO July 15, 2021 21:20
@willemneal willemneal changed the title fix: remove warning 232 fix: Make warning 232 pedantic Jul 22, 2021
@willemneal
Copy link
Contributor Author

@dcodeIO This should be good to go.

src/compiler.ts Outdated Show resolved Hide resolved
Copy link
Member

@dcodeIO dcodeIO left a comment

Choose a reason for hiding this comment

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

👍

@dcodeIO
Copy link
Member

dcodeIO commented Jul 22, 2021

Oh, wait, I think we also need to move the diagnostic to the pedantic range starting at 901 :)

@MaxGraey MaxGraey merged commit 3a33a25 into AssemblyScript:main Jul 23, 2021
@MaxGraey
Copy link
Member

Thanks!

@dcodeIO
Copy link
Member

dcodeIO commented Jul 23, 2021

I am not sure btw why you'd answer my request for this trivial change to keep numbering consistent with a confused emoji. But no problem, going to make the change myself :)

@willemneal
Copy link
Contributor Author

It was because I was confused at the time.

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