Skip to content

Do not return anything from Entity setters - #496

Merged
addshore merged 1 commit into
masterfrom
settersAreSetters
Jun 5, 2015
Merged

Do not return anything from Entity setters#496
addshore merged 1 commit into
masterfrom
settersAreSetters

Conversation

@thiemowmde

Copy link
Copy Markdown
Contributor

I realized this in #492 (comment).

I consider this a mistake that's not worth any documentation. I did a quick check in our code base and could not find a user relying on these return values.

The renamed method is unrelated, I just found this setter that's not a setter while searching for setters that may return something.

@thiemowmdethiemowmde added this to the 3.0 milestone Jun 3, 2015
@JeroenDeDauw

Copy link
Copy Markdown
Contributor

Rel notes do need updating. This bevahour has been there since 0.1 and was replied on back then.

@thiemowmdethiemowmde removed this from the 3.0 milestone Jun 3, 2015
@Benestar

Copy link
Copy Markdown
Contributor

Can somebody analyze our code base and check if anything still relies on this methods returning stuff? Otherwise it would be nice to have this in 3.0 as well.

Have to read comments in full... So +2 from my side if release notes are updated

@BenestarBenestar added this to the 3.0 milestone Jun 3, 2015
@thiemowmde

Copy link
Copy Markdown
ContributorAuthor

Release notes updated.

addshore added a commit that referenced this pull request Jun 5, 2015
Do not return anything from Entity setters
@addshore
addshore merged commit 268d900 into masterJun 5, 2015
@addshore
addshore deleted the settersAreSetters branch June 5, 2015 13:00
Comment threadRELEASE-NOTES.md

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is a method added to an interface a breaking change? I can't think of a way how does this could break existing code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imagine you implement the interface.

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

Development

Successfully merging this pull request may close these issues.

4 participants

@thiemowmde@JeroenDeDauw@Benestar@addshore