SI-6661 - Remove obsolete implicit parameter of scala.concurrent.promise method #1614

Merged
merged 1 commit into from Nov 16, 2012

Conversation

Projects
None yet
8 participants
@phaller
Contributor

phaller commented Nov 13, 2012

Review by @axel22

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@jsuereth

This comment has been minimized.

Show comment
Hide comment
@jsuereth

jsuereth Nov 14, 2012

Member

Is there a blocker ticket associated with this? Not sure if there was any discussion, but I'll give it a LGTM from a code perspective unless @adriaanm overrules.

Member

jsuereth commented Nov 14, 2012

Is there a blocker ticket associated with this? Not sure if there was any discussion, but I'll give it a LGTM from a code perspective unless @adriaanm overrules.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 14, 2012

Member

I'm not aware of any discussion nor a ticket. It's been in there like this for 11 months.
Granted, it wasn't officially released, but how can we expect people to use milestones and RCs for testing if we're then going to change public API at the last minute?

Member

adriaanm commented Nov 14, 2012

I'm not aware of any discussion nor a ticket. It's been in there like this for 11 months.
Granted, it wasn't officially released, but how can we expect people to use milestones and RCs for testing if we're then going to change public API at the last minute?

@retronym

This comment has been minimized.

Show comment
Hide comment
@axel22

This comment has been minimized.

Show comment
Hide comment
@axel22

axel22 Nov 14, 2012

Member

LGTM.

Member

axel22 commented Nov 14, 2012

LGTM.

@phaller

This comment has been minimized.

Show comment
Hide comment
@phaller

phaller Nov 14, 2012

Contributor

The associated ticket is SI-6661

Contributor

phaller commented Nov 14, 2012

The associated ticket is SI-6661

@heathermiller

This comment has been minimized.

Show comment
Hide comment
@heathermiller

heathermiller Nov 14, 2012

Member

(cross-posting)

This is an inconsistency introduced after refactoring implicit ExecutionContexts. In this commit: 1dfce90#diff-1

...the implicit ExecutionContexts were removed from everything else in Promise.scala, but it appears that method promise was missed in the scala.concurrent package object, which would've made sense to remove back then.

So this looks like a welcome correction.

Member

heathermiller commented Nov 14, 2012

(cross-posting)

This is an inconsistency introduced after refactoring implicit ExecutionContexts. In this commit: 1dfce90#diff-1

...the implicit ExecutionContexts were removed from everything else in Promise.scala, but it appears that method promise was missed in the scala.concurrent package object, which would've made sense to remove back then.

So this looks like a welcome correction.

@jsuereth

This comment has been minimized.

Show comment
Hide comment
@jsuereth

jsuereth Nov 14, 2012

Member

It's a new API and minimal change, I'm willing to let it in, but we do need to try to avoid these kinds of RC fixes in the future.

This one is a bit unique because of binary compatibility restrictions.

Member

jsuereth commented Nov 14, 2012

It's a new API and minimal change, I'm willing to let it in, but we do need to try to avoid these kinds of RC fixes in the future.

This one is a bit unique because of binary compatibility restrictions.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 14, 2012

Member

Ok. I think fixing is the right thing to do since it was an oversight, it should be mostly source-compatible and we wouldn't be able to fix it later. I'd like to emphasize what Josh said, though.

In future, when we need to fix things like this, please announce on the mailing lists first.

Member

adriaanm commented Nov 14, 2012

Ok. I think fixing is the right thing to do since it was an oversight, it should be mostly source-compatible and we wouldn't be able to fix it later. I'd like to emphasize what Josh said, though.

In future, when we need to fix things like this, please announce on the mailing lists first.

@gkossakowski

This comment has been minimized.

Show comment
Hide comment
@gkossakowski

gkossakowski Nov 14, 2012

Member

In this particular example the problem is not with the patch itself but with the commit message.

I do git archeology very often to avoid writing e-mails to people that worked on piece of code I'm trying to understand. For this process to be effective commit messages must clear and provide context. I'll point you to my own recent work: 90f12c4, ffc728c.

In 90f12c4 I didn't just say "We do not need to retransform classes once they are loaded." The next sentence explains why we do not to retransform those classes. The next paragraph explains what inspired me to introduce that change and what kind of problem it addresses.

Writing a good commit messages that make sense in a year is a long-term investment.

Once @heathermiller's comment is incorporated into commit message this change is ready to go.

Member

gkossakowski commented Nov 14, 2012

In this particular example the problem is not with the patch itself but with the commit message.

I do git archeology very often to avoid writing e-mails to people that worked on piece of code I'm trying to understand. For this process to be effective commit messages must clear and provide context. I'll point you to my own recent work: 90f12c4, ffc728c.

In 90f12c4 I didn't just say "We do not need to retransform classes once they are loaded." The next sentence explains why we do not to retransform those classes. The next paragraph explains what inspired me to introduce that change and what kind of problem it addresses.

Writing a good commit messages that make sense in a year is a long-term investment.

Once @heathermiller's comment is incorporated into commit message this change is ready to go.

SI-6661 - Remove obsolete implicit parameter of scala.concurrent.prom…
…ise method

Clarification of @heathermiller:
This is an inconsistency introduced after refactoring implicit ExecutionContexts.
In commit 1dfce90 the implicit ExecutionContexts were removed from everything else in Promise.scala,
but it appears that method promise was missed in the scala.concurrent package object, which would've made
sense to remove back then.
@phaller

This comment has been minimized.

Show comment
Hide comment
@phaller

phaller Nov 15, 2012

Contributor

Amended the commit message to include the clarifying comments of @heathermiller

Contributor

phaller commented Nov 15, 2012

Amended the commit message to include the clarifying comments of @heathermiller

adriaanm added a commit that referenced this pull request Nov 16, 2012

Merge pull request #1614 from phaller/issue/correct-promise-signature
SI-6661 - Remove obsolete implicit parameter of scala.concurrent.promise method

@adriaanm adriaanm merged commit 4de1a25 into scala:2.10.0-wip Nov 16, 2012

@gkossakowski

This comment has been minimized.

Show comment
Hide comment
Member

gkossakowski commented Nov 21, 2012

@phaller: thanks!

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