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

Warn if a variable is not updated in the setter function #63818

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heppocogne
Copy link
Contributor

Related to godot-proposals #4940

  • Add warning for when a member variable is not updated in the setter function.
  • Because annotation does not support setter/getter functions, this warning currently cannot be suppressed by @warning_ignore().

image

@heppocogne heppocogne requested a review from a team as a code owner August 2, 2022 10:09
Warn if a variable is not updated in the setter function

Add `SETTER_MISSING_ASSIGNMENT` warning
Fix formatting issues
@heppocogne heppocogne force-pushed the Warn-variable-not-updated-in-setter branch from 996c71f to a23dfe7 Compare August 2, 2022 10:17
@aaronfranke
Copy link
Member

What happens with this valid use case?

var milliseconds = 0.0

var seconds:
	get:
		return milliseconds / 1000.0
	set(value):
		milliseconds = value * 1000.0

@heppocogne
Copy link
Contributor Author

@aaronfranke
This PR gives a warning that seconds is not set in the setter function.

I think these codes should be warned in order to prevent mistakes, even if they are valid and works fine.
In addition, we can ignore warnings if they are unnecessary.

@jtnicholl
Copy link
Contributor

Personally I disagree, if the variable is not referenced by either the setter or getter as in the example above then it isn't created. That should not generate a warning, it's not likely to be a mistake in that case. If the warning only appears when it's used in one or the other, I could maybe get behind this.
Also,

In addition, we can ignore warnings if they are unnecessary.

But you said yourself above:

  • Because annotation does not support setter/getter functions, this warning currently cannot be suppressed by @warning_ignore().

IMO support for annotations on getters and setters should be added first, I don't like the idea of adding warnings that can't be suppressed.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@dalexeev
Copy link
Member

@adamscott adamscott modified the milestones: 4.1, 4.2 Jun 16, 2023
@adamscott
Copy link
Member

Post-poned the miletsone to 4.2 as this is an enhancement PR.

This PR needs a rebase too, as there's conflicts.

@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
@dalexeev dalexeev modified the milestones: 4.3, 4.4 Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants