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

Change rule G-7240 to cover IN OUT mode only #81

Closed
PhilippSalvisberg opened this issue Mar 5, 2020 · 5 comments · Fixed by #86
Closed

Change rule G-7240 to cover IN OUT mode only #81

PhilippSalvisberg opened this issue Mar 5, 2020 · 5 comments · Fixed by #86
Assignees
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@PhilippSalvisberg
Copy link
Collaborator

The title and the description of Rule G-7240 do not match. The description should concentrate on the combined mode IN OUT.

Title:_

G-7240: Avoid using an IN OUT parameter as IN or OUT only.

Description:

By showing the mode of parameters, you help the reader. If you do not specify a parameter mode, the default mode is IN. Explicitly showing the mode indication of all parameters is a more assertive action than simply taking the default mode. Anyone reviewing the code later will be more confident that you intended the parameter mode to be IN / OUT.

The implementation of the validator should change as well (currently it checks if IN or OUT is defined as mode). This lead to some confusion. See discussion in #46.

@PhilippSalvisberg PhilippSalvisberg added bug Something isn't working enhancement New feature or request labels Mar 5, 2020
@kibeha kibeha added this to the v4.0 milestone Aug 12, 2020
@kibeha kibeha self-assigned this Aug 12, 2020
@kibeha
Copy link
Collaborator

kibeha commented Aug 12, 2020

Seems to me the description of G-7240 fits more a general rule of "Always explicitly state parameter mode".
The title of G-7240 is more a matter of don't be lazy and choose IN OUT "just in case", but only use IN OUT if really needed.

I'm considering converting to two rules:

"Always explicitly state parameter mode."
With the reason from G-7240, but new examples.
(But I'm vacillating if this should go for functions too, as G-7440 states functions shouldn't use OUT, so letting functions use default instead of specifying IN could be more acceptable...)

"Avoid using an IN OUT parameter as IN or OUT only."
With updated reason/description.

Still thinking :-)

@PhilippSalvisberg
Copy link
Collaborator Author

Splitting sounds good. First rule can be checked easily.

"Avoid using an IN OUT parameter as IN or OUT only."
With updated reason/description.

This rule make sense. However, it is difficult to check with static code analysis. Maybe we should mark the guidelines where we do not plan to implement a check.

@kibeha
Copy link
Collaborator

kibeha commented Aug 12, 2020

Good idea to mark rules which are implemented/supported by tools/validators. I'll give it a thought if I can't semi-automate that.

So you say that rule G-7240 as described in the current Title ("Avoid using an IN OUT parameter as IN or OUT only.") is not currently implemented/checked by validator?

@PhilippSalvisberg
Copy link
Collaborator Author

This is an excerpt of the implementation. It checks "your" first rule only.

if (!param.isIn() && !param.isOut()) {
	warning(...);
}

@kibeha
Copy link
Collaborator

kibeha commented Aug 12, 2020

All the more sense to split the rule, since the implementation covers the current "Reason", but not the "Title" ;-)
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants