more appropriate on conditional

[gregoryyoung]

Was just reading through this code and saw the <> -1. >= is more appropriate as -2-int.MinValue are invalid.

[msftclas]

Hi @gregoryyoung, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

[KevinRansom]

Hi @gregoryyoung,

the documentation for the IndexOf api is very specific:

Reports the zero-based index of the first occurrence of a specified Unicode character or string within this instance. The method returns -1 if the character or string is not found in this instance.

https://msdn.microsoft.com/en-us/library/system.string.indexof(v=vs.110).aspx

I agree that both pieces of code will have the same effect. I'm not sure that one is more appropriate than the other.
It is true that if IndexOf future implementations decide to use other -ve values to signal error conditions we may have a bug.

Does that make sense?

Kevin

[gregoryyoung]

To be fair even having a conversation about this isn't worth the time. I
was just reading through the code and changed it and said well I guess I
should push that up. But yes its basically the difference between
explaining what you can use then using it vs what you can't use then using
anything else (explicit/implicit)

"It is true that if IndexOf future implementations decide to use other -ve
values to signal error conditions we may have a bug."

Normally I would avoid such couplings (this code has specified that index
of must not only keep its current contract but must also not change its
contact for anything that is unspecified in the future). Given in this case
IndexOf is unlikely to change its more one of those things you do out of
habit.

Cheers,

Greg

On Fri, May 1, 2015 at 8:09 PM, Kevin Ransom (msft) <
notifications@github.com> wrote:

Hi @gregoryyoung https://github.com/gregoryyoung,

the documentation for the IndexOf api is very specific:

Reports the zero-based index of the first occurrence of a specified
Unicode character or string within this instance. The method returns -1 if
the character or string is not found in this instance.

https://msdn.microsoft.com/en-us/library/system.string.indexof(v=vs.110).aspx

I agree that both pieces of code will have the same effect. I'm not sure
that one is more appropriate than the other.

It is true that if IndexOf future implementations decide to use other -ve
values to signal error conditions we may have a bug.

Does that make sense?

Kevin

—
Reply to this email directly or view it on GitHub
#399 (comment)
.

Studying for the Turing test

[mexx]

👍 for guarding by check for validity. It's more robust.
just my 2c

[latkin]

Given that the current code is correct and there is no bug, I don't see a motivation to make this change.

[gregoryyoung]

So no refactors should ever be submitted as a pull request?

[KevinRansom]

@gregoryyoung
Hi,

you may not follow the repo and issues as closely as some of our other contributors, and therefore I will refer you to a couple of our prior postings which may answer your questions.

1. Locking down the Visual F Codebase #371
2. Remaining work for F# 4.0 #386

From time to time we will modify our PR acceptance criteria due to the state of the repo or where in the project cycle we are situated. When that happens we will call it out to our contributors to ensure they use their time making valuable, timely contributions. We do not wish you to waste your time on PR's that will not be pulled any time soon, especially when we are getting ready to ship.

Right now as both posts state we will be pleased to accept bug fixes, where those fixes containing the minimum delta necessary to effect the change. We are minimizing risk to the product of unintended side effects.

Refactoring and code cleanup will happen when we have more time before the next release.

Even @dsyme is subject to this work flow, this PR File renamings and code cleanup #357 will not be accepted any time soon.

Thanks again

Kevin