Add NullParameterCheck, closes #49

[Gama11]

No description provided.

[Gama11]

Is there a convention for the naming of the option values? Sometimes they're UPPERCASE, (ENUM) sometimes lowercase (nl).

[adireddy]

I think this check should be called ParameterAssignmentCheck and either allow parameter assignment or not.

In Java world, parameter assignment is usually considered as a bad practice http://checkstyle.sourceforge.net/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/ParameterAssignmentCheck.html

The check can be customised and check for null assignments if parameter assignment is allowed.

Thoughts?

[adireddy]

Trying to keep the checks as close as possible to original checkstyle wherever possible http://checkstyle.sourceforge.net/checks.html

[Gama11]

I think that would be a bit confusing, parameter default values aren't really the same thing as reassigning them. Java doesn't even have parameter default values. I wouldn't consider having default values for parameters bad style either.

[Gama11]

Considering that, it's not really a deviation from the original checkstyle, it's just an additional, Haxe specific check.. The ParameterAssignmentCheck could still be added later.

[adireddy]

Yes in Java it checks for parameter assignment in the function body but we can use the same and tweak it for haxe.

For example, I don't like default assignment like this function test(i = 12, s = "bar") and always prefer making the parameters optional.

ParameterAssignmentCheck

• allowNullDefault - false

test(i = 12, s = "bar") - invalid
test(?i = 12, ?s = "bar") - valid
test(?i = 12, s = "bar") - invalid
test(?i = 12, s = null) - invalid
test(?i = 12, ?s) - valid
test(?i = 12, ?s = null) - invalid
test(i = 12, ?s) - invalid
test(i, ?s = "bar") - valid

The check should never allow either as it's equivalent to disabling the check.

We can change the name to ParameterDefaultCheck if you think ParameterAssignmentCheck is confusing.

[Gama11]

Regarding the first two valid examples:

test(?i = 12, ?s = "bar")
test(?i = 12, ?s)

These are bad code style IMO - you know about implied nullability? Because of the ?, you end up with Null<Int> which has a runtime overhead on some targets. See #86.

The either option still checks for redundancy (specifying both a null default value and marking the param as optional with ?):

// this is redundant
test(?s = null)

[adireddy]

You have answers for everything :)

Checkstyle is about code style and consistency so worrying about the runtime performance is up-to the user but it's important for the check to have all the options covered.

So if some targets have runtime overhead using ? then the check should have an option to not allow optional parameters at all.

ParameterAssignmentCheck

• allowDefaults - true/false
• allowNullDefault - true/false
• allowOptionals - true/false

Can we cover all the scenarios with the above options?

BTW totally off topic, I can add you as a collaborator to this repo if you are interested. Let me know.

[Gama11]

Just to be clear, optionals are only problematic on types that are not already nullable (i.e. Int, Float and Bool on static targets). I don't think disallowing optional parameters in general makes sense (at least for this case, maybe for style preference), you'd have to check if the argument has one of these three types.

I think the check should be called ParameterDefaultCheck, not ParameterAssignmentCheck to avoid confusion like you suggested earlier. So you think both #86 and #49 should be covered with a single check?

Regarding the options:

• allowDefaults and allowNullDefault seem reasonable.
• allowOptionals - for the case I mentioned, this would have to be allowOptionalBasicTypesWithDefaultValueDifferentFromNull. :P The implied nullability is only really relevant for basic types. There are still valid use cases for nullable basic types though, Flixel has these in a few places for example. It only becomes pointless when you have a nullable basic type with a non-null default value - like I described in Add a check for arguments with optional basic types and a non-null default #86, in that case the user probably didn't know about implied nullability. Because of how specific this is, I originally suggested having a separate check for this.

Being added as a collaborator could be cool. I can't make any promises regarding my activity in the future though.

[adireddy]

You are now a collaborator 👍

Let's go with ParameterAssignmentCheck with allowDefaults and allowNullDefault for now.

You can merge whenever you are happy with this check.

[AlexHaxe]

Welcome to the team!

[Gama11]

Thanks. :)

Are you sure about the name? Like I mentioned, this is not really about assignment, it's about default values.

What happens to the redundancy check (specifying both = null and ?)? Is that always run like I have it implemented right now, or is it simply covered by allowNullDefaults = false?

[adireddy]

Sorry my bad, I mean to say ParameterDefaultCheck with allowDefaults and allowNullDefault.

[adireddy]

I think allowNullDefaults = false? should cover redundancy check (specifying both = null and ?)

[adireddy]

I have started documenting all the checks sometime back. If you guys have gitbboks account I can add as collaborators there as well.

https://adireddy.gitbooks.io/haxe-checkstyle/content/

[Gama11]

@adireddy I now do, signed up with GitHub so same account name.

[AlexHaxe]

I signed up for an account when you moved docs to gitbook, but it didn't allow me to edit, so I didn't pursue it further.

[adireddy]

Awesome. Just added both of you.

[Gama11]

Gonna start with a fresh branch for this.