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

Alllow multi character type variable names #183

Merged
merged 1 commit into from Jun 2, 2019

Conversation

Projects
None yet
4 participants
@dain
Copy link
Member

commented Jun 2, 2019

No description provided.

@martint

martint approved these changes Jun 2, 2019

@findepi

findepi approved these changes Jun 2, 2019

Copy link
Contributor

left a comment

yay!

</module>
<module name="MethodTypeParameterName">
<property name="format" value="^[A-Z][0-9]?$" />
<property name="format" value="^[A-Z][A-Z0-9]*$" />

This comment has been minimized.

Copy link
@findepi

findepi Jun 2, 2019

Contributor

In the past I found it convenient to name my params camel case rather than all-caps (in the case where single-char param names were not enough).
Since we now are moving from single-char (plus optional number, whatever) to multi-char, we can make any choice we want.

This comment has been minimized.

Copy link
@dain

dain Jun 2, 2019

Author Member

I think it might be confusing to mix the type variable and normal class name namespace. I could imagine wanting an underscore in some cases.... I suggest that we expand this more when we find another use case.

BTW my use case was for a between function, I wanted to use <V, MIN, MAX>

This comment has been minimized.

Copy link
@martint

martint Jun 2, 2019

Member

In the past I found it convenient to name my params camel case rather than all-caps (in the case where single-char param names were not enough).

Yeah, I like doing that, too. For example, I once was working on a library to manipulate graphs, and I used NodeType and EdgeType as my type variables.

This comment has been minimized.

Copy link
@martint

martint Jun 2, 2019

Member

I think it might be confusing to mix the type variable and normal class name namespace.

It is the same namespace, though. Type variables have precedence over class names, just like local variables have precedence over instance variables. IDEs like IntelliJ will highlight them differently.

This comment has been minimized.

Copy link
@findepi

findepi Jun 2, 2019

Contributor

So it's 2-1 :)

This comment has been minimized.

Copy link
@dain

dain Jun 2, 2019

Author Member

@martint yes, it is the same namespace, which is why it can be confusing since you can shadow types. Anyway, I don't feel strongly. I just have a preference for relaxing when we need stuff.

This comment has been minimized.

Copy link
@electrum

electrum Jun 3, 2019

Member

The IDE does highlight generic parameters differently, though that doesn't help on GitHub, etc. The advantage of all uppercase is that you know visually it can't be a class name (at least one that follows our naming convention).

This comment has been minimized.

Copy link
@martint

martint Jun 3, 2019

Member

The IDE does highlight generic parameters differently, though that doesn't help on GitHub, etc. The advantage of all uppercase is that you know visually it can't be a class name (at least one that follows our naming convention).

But that's the same argument we've made for ages against having a naming convention for local variables vs instance methods like many projects do (not that I'm advocating for that!). It's easy to spot them in an IDE, but not in github, etc.

@findepi

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

typo in cmt msg "Alllow"

@dain dain force-pushed the dain:multi-char-type-variables branch from 7debbf2 to 57e3b5c Jun 2, 2019

@dain dain merged commit 57e3b5c into airlift:master Jun 2, 2019

@dain dain deleted the dain:multi-char-type-variables branch Jun 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.