-
Notifications
You must be signed in to change notification settings - Fork 528
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
Make sure method return non-null values #413
base: master
Are you sure you want to change the base?
Make sure method return non-null values #413
Conversation
4c3f574
to
4ad786c
Compare
@wimvelzeboer, can you also please check |
4ad786c
to
5dbfb3a
Compare
@wimvelzeboer, the
Can you take a look at this please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @ImJohnMDaniel, @stohn777, and @wimvelzeboer)
a discussion (no related file):
Due to the changes in behavior that this PR proposes, my vote is to reject it
sfdx-source/apex-common/main/classes/fflib_SObjects.cls
line 169 at r2 (raw file):
Previously, wimvelzeboer (William Velzeboer) wrote…
@ImJohnMDaniel You are right indeed! Its changed!
I'm less concerned about the breaking change that this introduces since you're asking for Id values and those should be non-null. But the risk isn't zero that it will break someone's assumption of how this method works.
sfdx-source/apex-common/main/classes/fflib_SObjects.cls
line 188 at r4 (raw file):
for (SObject record : getRecords()) { if (String.isBlank((String) record.get(field)))
I don't know if I agree with this change -- Blank strings and null are distinct values for a String field. The Apex doc comment nor does the method name make it clear that values consisting of whitespace only will be excluded.
This change is definitely a breaking change and not worth the risk, IMO.
After upgrading my projects to the latest fflib-apex-common, where some methods were moved from fflib-apex-extensions to fflib-apex-common, I found that a high number of unit-tests were failing.
Then I found out that the
getIdFieldValues
andgetStringFieldValues
were suddenly also returning null values, which makes no sense. This fix will make sure these methods always return the values and omit the nullsThis change is