Skip to content
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

Removed unnecessary assertion on boolean values #20910

Merged
merged 4 commits into from Oct 13, 2016

Conversation

Projects
None yet
3 participants
@kunal642
Copy link
Contributor

commented Oct 13, 2016

No description provided.

@@ -113,7 +113,7 @@ public synchronized int getNumberOfInFlightFetches() {
nodesToIgnore.addAll(ignoreNodes);
fillShardCacheWithDataNodes(cache, nodes);
Set<NodeEntry<T>> nodesToFetch = findNodesToFetch(cache);
if (nodesToFetch.isEmpty() == false) {
if (!nodesToFetch.isEmpty()) {

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 13, 2016

Contributor

We explicitly prefer false == or == false forms over !.

This comment has been minimized.

Copy link
@kunal642

kunal642 Oct 13, 2016

Author Contributor

Thats ok, but do you also prefer "== true"??

This comment has been minimized.

Copy link
@kunal642

kunal642 Oct 13, 2016

Author Contributor

I have reverted the changes to "== false"
Please review them.

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 13, 2016

Contributor

Yeah, == true is fairly weird. I mean, if you are doing it to line up with some other formatting, maybe.... But, yeah, it is usually wrong. I'll have a look.

@nik9000
Copy link
Contributor

left a comment

I left a few minor things and I think I spotted one mistake.

Settings.Builder persistentSettings = Settings.builder();
persistentSettings.put(currentState.metaData().persistentSettings());
changed |= clusterSettings.updateDynamicSettings(persistentToApply, persistentSettings, persistentUpdates, "persistent");

boolean changed = clusterSettings.updateDynamicSettings(transientToApply, transientSettings, transientUpdates, "transient") |

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 13, 2016

Contributor

I think this was more readable the old way.

This comment has been minimized.

Copy link
@kunal642

kunal642 Oct 13, 2016

Author Contributor

Why do the OR operation on 2 methods in 3 lines when we can simply do the same in one line.
In the earlier way we had to do the OR operation twice.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Oct 13, 2016

Member

I agree with @nik9000, it's more readable as it is currently.

This comment has been minimized.

Copy link
@kunal642

kunal642 Oct 13, 2016

Author Contributor

@nik9000 reverted the changes

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 13, 2016

Contributor

This isn't a hot code path so I'm quite willing to try and write the most readable code possible and let the optimizer try and optimize out the extra OR operation if it is capable of it.

@@ -452,7 +452,7 @@ public static GeoDistanceSortBuilder fromXContent(QueryParseContext context, Str
geoDistance = GeoDistance.fromString(parser.text());
} else if (parseFieldMatcher.match(currentName, COERCE_FIELD)) {
coerce = parser.booleanValue();
if (coerce == true) {
if (!coerce) {

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 13, 2016

Contributor

This one is changed.

This comment has been minimized.

Copy link
@kunal642

kunal642 Oct 13, 2016

Author Contributor

Corrected

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 13, 2016

Contributor

Thanks.

assert fetching == true : "restarting fetching, but not in fetching mode";
assert valueSet == false : "value can't be set when restarting fetching";
assert fetching : "restarting fetching, but not in fetching mode";
assert valueSet == false: "value can't be set when restarting fetching";

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 13, 2016

Contributor

Minor nit: you changed the formatting. We don't really have a standard for formatting beyond line length, and even that isn't globally enforced because we had a thousand files that violated it. But you may as well add the space back before the : so it looks like the code above it.

This comment has been minimized.

Copy link
@kunal642

kunal642 Oct 13, 2016

Author Contributor

changed to the default formatting

This comment has been minimized.

Copy link
@nik9000

nik9000 Oct 13, 2016

Contributor

Thanks.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2016

@elasticmachine, test this please.

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2016

I labeled this non-issue because it doesn't make any outward facing changes that folks who read the change lists will need to read.

kunal642 added some commits Oct 13, 2016

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2016

OK! I'll have a look at merging this in a bit.

@nik9000 nik9000 merged commit e20d9d6 into elastic:master Oct 13, 2016

1 check passed

CLA Commit author has signed the CLA
Details
@nik9000

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2016

I tested this locally and everything passed as expected. I'm testing a cherry-pick into the 5.x branch as well.

nik9000 added a commit that referenced this pull request Oct 13, 2016

Removed unnecessary assertion on boolean values (#20910)
* Removed unnecessary assertion on boolean values

* Reversed changes for false assertion

* corrected formatting

* reverted changes for SettingsUpdater
@nik9000

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2016

Master: e20d9d6
5.x: 16689f7

@nik9000 nik9000 added the v5.1.1 label Oct 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.