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

Enabled errors on unused variables. #4457

Merged
merged 1 commit into from
Nov 23, 2023
Merged

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Nov 23, 2023

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.


This change is Reviewable

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)

a discussion (no related file):
Errors? Has this been discussed with the community members, and has feedback been gathered about this idea?

I believe most languages implement this as warnings; the only language I know that emits fatal errors is Golang, so I expect this to be a rather unexpected behaviour.

My personal feedback is that this should be a warning, as I tend to often leave unfinished code and run tests over it. But I'm happy to hear others voice!


Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 53 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @mkaput)

a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

Errors? Has this been discussed with the community members, and has feedback been gathered about this idea?

I believe most languages implement this as warnings; the only language I know that emits fatal errors is Golang, so I expect this to be a rather unexpected behaviour.

My personal feedback is that this should be a warning, as I tend to often leave unfinished code and run tests over it. But I'm happy to hear others voice!

since it is very easy to prevent the error (just adding an _ before) - I see no reason for this to not be an error.
warning are currently not a concept we have - and i see no reason to add it just for that.


Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 53 of 53 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput)

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

a discussion (no related file):

Previously, orizi wrote…

since it is very easy to prevent the error (just adding an _ before) - I see no reason for this to not be an error.
warning are currently not a concept we have - and i see no reason to add it just for that.

I understand the implementation reasons for this, but I just want to say, that even this simple fix with _ requires manual work in the code. It will have a tendency to kick the developer out of their flow of thought (for example, in the scenario I outlined in the previous comment). That's a subtle UX inconvenience, that may affect users' perception of the language.

I, honestly, strongly suggest gathering community feedback before releasing this.


@milancermak
Copy link
Contributor

100% agree with what Marek said. This shouldn't be an error.

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput)

a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

I understand the implementation reasons for this, but I just want to say, that even this simple fix with _ requires manual work in the code. It will have a tendency to kick the developer out of their flow of thought (for example, in the scenario I outlined in the previous comment). That's a subtle UX inconvenience, that may affect users' perception of the language.

I, honestly, strongly suggest gathering community feedback before releasing this.

makes sense - pre-adding a warning concept.


Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput)

a discussion (no related file):

Previously, orizi wrote…

makes sense - pre-adding a warning concept.

happens in the following PR.
the PR after it will be removing the edition dependency from it.
scarb wrapper will need to support skipping the diagnostics for these cases.


@orizi orizi force-pushed the pr/orizi/map-lowering-diags/372bf7f6 branch from 451d9d3 to 584ec65 Compare November 23, 2023 11:45
Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

a discussion (no related file):

scarb wrapper will need to support skipping the diagnostics for these cases.

I don't fully follow, but I expect this to get more clear when I'll see the code. If DiagnosticReporter gets the information about the level of the diagnostic, we will print them as warn:. The question is, what should be the out coming Result of the compilation. I think perhaps we could introduce a custom enum: Ok, Warnings, Err here.


Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 53 of 53 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@orizi orizi added this pull request to the merge queue Nov 23, 2023
Merged via the queue into main with commit 32e653f Nov 23, 2023
@orizi orizi deleted the pr/orizi/map-lowering-diags/372bf7f6 branch November 26, 2023 11:36
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.

4 participants