Merge openfsharp 1 #255
Merge openfsharp 1 #255
Conversation
enricosada
commented
Feb 20, 2015
enricosada
commented
- e01e100 merge fix test, now is crossplatform (see commit message)
- 655df3d cleanup style for better diff and merge of some changes
|
Thanks for taking care of this Enrico, it is a valuable thing to do. The changes look good except for the additional try/with, I would like to understand why it was necessary in the open codebase before we take it. Thanks Kevin |
| @@ -104,7 +104,7 @@ type LanguagePrimitivesModule() = | |||
|
|
|||
| // reference type | |||
| let resultRef = LanguagePrimitives.GenericComparison "ABC" "ABCDE" | |||
| Assert.AreEqual(resultRef,-68) | |||
| Assert.AreEqual(sign resultRef,-1) | |||
There was a problem hiding this comment.
Hmm, not a blocker, but the nunit APIs are authored to expect Assert.AreEqual(expected, actual) , whereas these are called the other way Assert.AreEqual(actual, expected). The failure message will be a little bit confusing.
Doesn't really matter, they were already wrong. In the future for new tests we can try to remember this though.
There was a problem hiding this comment.
let shouldEqual (expected:'a) (actual:'a) = Assert.AreEqual(expexted,actual)
Is used in many projects and also enforces the correct type on compile time
There was a problem hiding this comment.
(the benefit is in using |>)
There was a problem hiding this comment.
good catch! i'll fix it, easy
A long time ago I was filling a bug report to NUnit about this, only to find the parameters were reversed 😅
|
Agree with @KevinRansom. Looks great, but would want to know the origin of the exception handling. |
|
IIRC it was because the simulated registry is not available on Mono in all circumstances and attempts to access it can fail. So I don't really think the change should be brought into VF# as is. While changes like this (which myself and others put in) served a purpose a few years ago under a "get it running on Mono/Linux/Mac at all costs" agenda, they don't seem appropriate now, especially since we're very likely to have to add even another version of cross-platformness to this repo as we consider bringing the F# compiler up on cross-platform editions of .NET Core. So I'd vote for keeping the try/catch out for now, but to continue to minimize the textual diffs to reduce them to a small known set of items, and perhaps documenting those. |
|
yes, i was thinking that was ok because the current code do pretty much the same ( see https://github.com/Microsoft/visualfsharp/blob/fac44095a8dbc7303efa9866e0e2d9dddbdeae5d/src/utils/CompilerLocationUtils.fs#L79-L85 ). i'll remove the next time i'll separate whitespace cleanup and change of behavior. |
enricosada
force-pushed
the
merge_openfsharp_1
branch
from
655df3d to
d385f25
Compare
| @@ -202,11 +202,14 @@ module internal FSharpEnvironment = | |||
| // Check for an app.config setting to redirect the default compiler location | |||
| // Like fsharp-compiler-location | |||
| try | |||
| // FSharp.Compiler support setting an appkey for compiler location. I've never seen this used. | |||
|
Looks ok to me. |
closes #255 commit d385f25 Author: enricosada <[email protected]> Date: Mon Feb 23 15:50:35 2015 +0100 fix Assert.AreEqual arguments (first is expected value, second is actual value) commit 172c363 Author: enricosada <[email protected]> Date: Fri Feb 20 18:35:54 2015 +0100 merge openfsharp changes commit aea137d Author: enricosada <[email protected]> Date: Fri Feb 20 17:24:23 2015 +0100 fix test the ordinal string comparison is implementation specific, only checks with less than 0, 0 and greater than 0 are meaningful
|
This is applied |
