close
Skip to content

fix: Make warning 232 pedantic - #1986

Merged
MaxGraey merged 6 commits into
AssemblyScript:mainfrom
willemneal:fix/remove_warning_232
Jul 23, 2021
Merged

fix: Make warning 232 pedantic#1986
MaxGraey merged 6 commits into
AssemblyScript:mainfrom
willemneal:fix/remove_warning_232

Conversation

@willemneal

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@dcodeIO

dcodeIO commented Jul 15, 2021

Copy link
Copy Markdown
Member

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

@willemneal

Copy link
Copy Markdown
Contributor Author

Oh you're right!

@willemneal

Copy link
Copy Markdown
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

dcodeIO commented Jul 15, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

@dcodeIO This should be good to go.

Comment thread src/compiler.ts Outdated

@dcodeIO dcodeIO left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@dcodeIO

dcodeIO commented Jul 22, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

Thanks!

@dcodeIO

dcodeIO commented Jul 23, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
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