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

Handle framework identifiers comparison (WIP) #1127

Closed
wants to merge 3 commits into from

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Oct 11, 2015

FrameworkIdentifier instances were compared structurally before but it was problematic in some cases as >= net45 matched sl40 that wasn't in the same framework.

This PR add new operators (.>, .>=, .<, .<=) and methods (Min, Max, IsSameFramework) to FrameworkIdentifier that do the comparison with the expected semantic.

Fixes #1124

Questions

This PR is a work in progress because I have a few questions about the way I did the change.

Structural Equality

The original problem come from the fact that structural comparison was used for FrameworkIdentifier.
But in fact no comparison (that could be used for ordering) could ever be correct for this type because net40 > sl40 should return false but net40 < sl40 should return false too.

Problem with that is that Sets are used over FrameworkIndentifier and due to their implementation require comparison... and I don't see any clean easy way out. Solutions that came to mind are :

  1. Adding a immutable Set structure that only require equality and use it for FrameworkIdentifier instead of the built-in one
  2. Use the HashSet structure from the framework, it's less "F#" but doesn't require comparison (Potentially with a small helper making it's usage more "F#")
  3. Keep structural comparison in addition to the semantic one, the risk is that any new code would need to be written carefully as the standard operators would be present but act in a very un-intuitive way

In the end I applied 3 as the rest of the changes were too big (Set is used pretty often and FrameworkIdentifier is part of other types that are themselves used in Sets) but I wonder if it's the correct decision.

isTargetMatchingRestrictions

I extracted this method to be able to test it but it's behavior relative to portable profiles is pretty specific and I don't know if the name is specific enough.

vbfox added 3 commits October 11, 2015 14:15
FrameworkIdentifier instances were compared structurally before but it
was problematic in some cases as '>= net45' matched 'sl40' that wasn't
in the same framework.

This commit add new operators (.>, .>=, .<, .<=) and methods (Min, Max,
IsSameFramework) to FrameworkIdentifier that do the comparison with the
expected semantic.

Fixes fsprojects#1124
@forki
Copy link
Member

forki commented Oct 12, 2015

would #1130 be enough?

@vbfox vbfox closed this Oct 15, 2015
@vbfox vbfox deleted the silverlight-dotnet-framework branch October 15, 2015 14:10
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.

2 participants