Skip to content

Easier compare of open fsharp#252

Closed
enricosada wants to merge 1 commit intodotnet:masterfrom
enricosada:merge_openfsharp_style_only
Closed

Easier compare of open fsharp#252
enricosada wants to merge 1 commit intodotnet:masterfrom
enricosada:merge_openfsharp_style_only

Conversation

@enricosada
Copy link
Copy Markdown
Contributor

this merge some changes from openfsharp repo

the whitespace cleanup is done to make easier to compare files of the openfsharp repo
now all differences with the src directory are pretty much bugfix, features or infrastructure

There is also the parallel pr fsharp/fsharp#385 to openfsharp repo

@enricosada enricosada changed the title merge changes of open fsharp repo Easier compare of open fsharp Feb 19, 2015
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.

Why is there a difference here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you should download fsharp/fsharp#385 and do a file compare of src directory.
That change make easier to diff codebase, is harmless, i can remove it if not usefull enough

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.

We are happy to accept sensible whitespace cleanup, but if this type annotation isn't needed, the open repo should remove it.

@enricosada
Copy link
Copy Markdown
Contributor Author

btw, some files differ only because of encoding ( UTF-8 vs UTF-8 with bom, etc ), usually app.config or .resx files

is this a good way to start merge some part of openfsharp repo?

i think we can start diff files, split by features ( + test ) and send pr here, right?

@KevinRansom
Copy link
Copy Markdown
Contributor

I am okay with pulling this request. Although I notice we have foo and bar in the test code. I will add an issue to remove those. I also wonder why it was necessary to modify the signature of EvaluateQuotation in fsharp/fsharp.

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.

We can't change this back, as it introduces the string "barf" to the codebase. 😆 We had to change this prior to open sourcing on Codeplex because internal code scrub tools complained. Would recommend open edition repo accepts our version.

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.

that's a joke, right?

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.

I wish it were

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.

@forki we ship products around the globe and have developed processes to protect us against inadvertently offending the sensibilities of our customers. Foo + Bar is one of the more minor examples of things we look for. Because it represents curse-words that some although not all may find offensive.

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.

I understand that, but this is a false positive. Changing code because a false positive of a tool is [insert curse-word] ;-)

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 particular one is about "barf", not foo+bar. IIRC, a surprisingly wide array of accidental bodily fluid references had to be removed... 💦

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.

but it's not "barf". it's "barfoobaz" ;-)
I don't thing this word exists at all. If your tool is complaining about infixes with barf then the german word "barfrau" (female barkeeper) would never be used inside of MS!?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@latkin Am I right in assuming that e.g. isPermanent posed a problem? ;)

@latkin
Copy link
Copy Markdown
Contributor

latkin commented Feb 19, 2015

Seems like sensible cleanup for the most part. Only objections are the test string change and the type annotation - the version here should be ported to the open repo for those (unless the annotation is somehow required for mono?)

@KevinRansom "foo" and "bar" are fine with PoliCheck, as long as they aren't together to make "foobar". No need to remove them AFAIK.

@enricosada
Copy link
Copy Markdown
Contributor Author

Ok, i'll remove them from there and push it to open fsharp pr

@KevinRansom
Copy link
Copy Markdown
Contributor

So I am seeing these 4 failures testing this PR that I don't recognize when I run the tests on the fsharp4 branch

fsharp\core\queriesOverOData -- failed
TypeProviders\BuiltIn\OdataService\StaticParam (DataServiceCollection_false.fs) -- failed
TypeProviders\BuiltIn\OdataService\StaticParam (DataServiceCollection_true.fs) -- failed
TypeProviders\BuiltIn\OdataService\StaticParam (DataServiceCollection_default.fs) -- failed

you can repro this by:

cd tests
runtests.cmd release fsharp

and
runtests.cmd release fsharpqa

@latkin
Copy link
Copy Markdown
Contributor

latkin commented Feb 19, 2015

What does the failure log say? (in tests\testresults\fsharp_failures.log)

@KevinRansom
Copy link
Copy Markdown
Contributor

Hmm now they pass, I must have had a slight configuration issue:

fsharp\core\queriesOverOData -- passed
TypeProviders\BuiltIn\OdataService\StaticParam (DataServiceCollection_false.fs) -- passed
TypeProviders\BuiltIn\OdataService\StaticParam (DataServiceCollection_true.fs) -- passed
TypeProviders\BuiltIn\OdataService\StaticParam (DataServiceCollection_default.fs) -- passed

Sorry for the randomization

@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented Feb 19, 2015

@enricosada - good to have this, thanks

@rojepp
Copy link
Copy Markdown
Contributor

rojepp commented Feb 19, 2015

Just a general Q, why not merge fharp/fsharp and Microsoft/visualfsharp together right now (other than barf and other profanities 😆)? It just seems like overhead?

@KevinRansom
Copy link
Copy Markdown
Contributor

We will do what we can to align visualfsharp with the fsharp\fsharp repo, so long as it does not destabilize our process. Making the delta as small as possible seems like a good plan. I will leave maintenance or otherwise of the fsharp\fsharp repo to it's owners to decide about.

However, I note that it has been a significant repo in the F# ecosystem and has served as the root of the cross platform development effort. It does not seem to me that it needs to go away, and the overhead of maintaining it can be made very small.

@enricosada
Copy link
Copy Markdown
Contributor Author

i want to merge the two repo.

this is the preliminary work, to easier understand the differences (to me and others).
now are easy to spot, very little noise (against the other pr codebase)

i'll add an issue with summary of all the differences, so we can split, review and import them one by one (easier iterative than big bang)

@enricosada enricosada force-pushed the merge_openfsharp_style_only branch from 0d40b70 to ae8fd8d Compare February 20, 2015 08:59
@enricosada
Copy link
Copy Markdown
Contributor Author

fixed as comments (farbazz and useless signature)

@enricosada
Copy link
Copy Markdown
Contributor Author

Hmm now they pass, I must have had a slight configuration issue:

fsharp\core\queriesOverOData -- passed

@KevinRansom see #90 , the core/queriesOverOData are flaky, i had errors if the test cannot download data from internet. Or maybe yours is another problem 😄 but i think is nice to know these are flaky, easy rerun it

@dsyme
Copy link
Copy Markdown
Contributor

dsyme commented Feb 20, 2015

Aligning the repos as closely as possible is very important and valuable work.

I don't expect the repos to be fully merged any time soon. For starters, it is in the mission statement of the FSSF to maintain the fsharp/fsharp repo, and it is one way that the organization commits to the permanent open, cross-platform existence of F#. Also, the MS repo is just under different constraints - e.g. requiring a CLA. Finally, the fsharp/fsharp repo has traditionally done a few more things that VF# - e.g. cross-platform compiler and the FSharp.Core nuget packages are there. These technical things can gradually converge however.

Cheers
Don

latkin pushed a commit that referenced this pull request Feb 21, 2015
@latkin
Copy link
Copy Markdown
Contributor

latkin commented Feb 21, 2015

This is applied (GH doesn't auto-close until fix commit is merged to "primary" branch).

@latkin latkin closed this Feb 21, 2015
@enricosada
Copy link
Copy Markdown
Contributor Author

@dsyme yes, maybe converge/align is better at explain what i am doing, instead of merge

I dont want to discuss/change the current fork of repos, only port all (often small or cosmetics) changes so it is easier to diff repos

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants