Skip to content

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.

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