Using case objects in place of case classes having no members

[nurkiewicz]

Idiomatic Scala uses case objects in favor of empty case class.

[hekonsek]

+1 for Tomek's pull request

[hekonsek]

I'm just not sure whether do we really need to bind the variable in the following line in TransactionSynchronizationManagerTests:

case e@AfterCommitEvent => afterEvent = e

What is is purpose of this change?

[nurkiewicz]

Do you suggest:

case AfterCommitEvent => afterEvent = AfterCommitEvent

Indeed, I was misled by previous case. I'll commit this to pull branch if you agree.

[hekonsek]

Yeah, this binding is a little bit confusing in this context :) .

Actually with your case object under the hood we could even consider using boolean variables in this test. For example:

test("Should match multiple events.") {
    var beforeEventReceived: Boolean = false
    var afterEventReceived: Boolean = false
    transactional() {
      status => {
        TransactionSynchronizationManager.registerSynchronization {
          case BeforeCommitEvent => beforeEventReceived = true
          case AfterCommitEvent => afterEventReceived = true
        }
      }
    }
    beforeEventReceived should be (true)
    afterEventReceived should be (true)
}

What do you think?

[nurkiewicz]

Looks cleaner, however BeforeCommitEvent actually conveys some information (readOnly flag), so maybe:

  test("Should match multiple events.") {
    var beforeEvent: Option[BeforeCommitEvent] = None
    var afterEventReceived = false
    transactional() {
      status => {
        TransactionSynchronizationManager.registerSynchronization {
          case e: BeforeCommitEvent => beforeEvent = Some(e)
          case AfterCommitEvent => afterEventReceived = true
        }
      }
    }
    beforeEvent should be (Some(BeforeCommitEvent(false)))
    afterEventReceived should be (true)
  }

What I don't like is the lack of symmetry - your suggestion looks better from that perspective.

[hekonsek]

The purpose of this partocular test is to validate that multiple events can be caught by single registration, so I personally would prefer clean boolean variables over strict testing here.

[nurkiewicz]

If the test only makes sure given events were triggered (moreover order is not important), I agree, Boolean is enough and more expressive. I'll rewrite it later today.

[poutsma]

Looks good to me. Thanks for contributing! I will merge as soon as you're done with it.

[nurkiewicz]

Done. Unfortunately case classes and case objects are handled slightly different in pattern matching:

case _: BeforeCommitEvent => beforeEventReceived = true
case AfterCommitEvent => afterEventReceived = true

[hekonsek]

I personally prefer the following syntax for matching all classes.

case BeforeCommitEvent(_) => beforeEventReceived = true

But its up to the spring-scala users to decide how they would like to use the library.

I think that pull request is ready to merge in the current state :) .

[poutsma]

Merged. Thanks for contributing!