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

Style convention for override modifier #291

Open
ceedubs opened this Issue Apr 24, 2015 · 5 comments

Comments

Projects
None yet
4 participants
@ceedubs
Member

ceedubs commented Apr 24, 2015

We should choose a convention for when we use the override modifier and be consistent with it.

See this discussion.

We can use the comments section of this issue page to discuss what we want our convention to be. Once we've reached a conclusion, someone can create a follow-up PR that updates code to follow the established convention.

@ceedubs

This comment has been minimized.

Show comment
Hide comment
@ceedubs

ceedubs May 5, 2015

Member

I think I might have just found another benefit of using override everywhere it is allowed. This comment made me look into whether we could have Scalastyle enforce that we have return types on public methods. It can, but this would probably require us to add explicit return types in a bunch of places where we are providing an implementation for an abstract method. If we were to add override in those places, we could use Scalastyle's ignoreOverride flag to not enforce the explicit return type in these cases.

There may be an argument that it's better to just enforce explicit return types for all public methods whether they are implementing an abstract method or not.

Member

ceedubs commented May 5, 2015

I think I might have just found another benefit of using override everywhere it is allowed. This comment made me look into whether we could have Scalastyle enforce that we have return types on public methods. It can, but this would probably require us to add explicit return types in a bunch of places where we are providing an implementation for an abstract method. If we were to add override in those places, we could use Scalastyle's ignoreOverride flag to not enforce the explicit return type in these cases.

There may be an argument that it's better to just enforce explicit return types for all public methods whether they are implementing an abstract method or not.

@non

This comment has been minimized.

Show comment
Hide comment
@non

non May 5, 2015

Member

Yeah, I pretty much think the type annotations should be on overridden methods too.

Member

non commented May 5, 2015

Yeah, I pretty much think the type annotations should be on overridden methods too.

@ceedubs

This comment has been minimized.

Show comment
Hide comment
@ceedubs

ceedubs May 5, 2015

Member

Okay. I'll look into submitting a PR for that.

Member

ceedubs commented May 5, 2015

Okay. I'll look into submitting a PR for that.

@stew

This comment has been minimized.

Show comment
Hide comment
@stew

stew Jun 5, 2015

Member

personally I'm in favor of putting it everywhere it is allowed, it just enables more compiler verification of types. I don't feel strongly enough to go to the matt with someone on it though

Member

stew commented Jun 5, 2015

personally I'm in favor of putting it everywhere it is allowed, it just enables more compiler verification of types. I don't feel strongly enough to go to the matt with someone on it though

@sritchie

This comment has been minimized.

Show comment
Hide comment
@sritchie

sritchie Sep 21, 2016

Contributor

Looks like this is closed by #303, yeah?

Contributor

sritchie commented Sep 21, 2016

Looks like this is closed by #303, yeah?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment