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?

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

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.

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.

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.

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)

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.

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.

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.

Welcome to the team!

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?

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

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

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/

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

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.

Awesome. Just added both of you.

Gonna start with a fresh branch for this.