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

Versioning_Engine: method to easily generate method keys for versioning #1693

Merged
merged 4 commits into from
Apr 24, 2020

Conversation

adecler
Copy link
Member

@adecler adecler commented Apr 23, 2020

Issues addressed by this PR

Provide helper method/component to go along with this PR: BHoM/BHoM_UI#247

image

This makes it way easier to populate the inputs of a PreviousVersion attribute.

@adecler adecler requested review from FraserGreenroyd, IsakNaslundBh, kThorsager and al-fisher and removed request for al-fisher April 23, 2020 07:56
@adecler adecler self-assigned this Apr 23, 2020
@adecler adecler added the type:feature New capability or enhancement label Apr 23, 2020
@adecler adecler added this to the BHoM 3.2 β MVP milestone Apr 23, 2020
Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

Still reviewing functionality wise, but a few comments to be starting with? 😄

Versioning_Engine/Versioning_Engine.csproj Outdated Show resolved Hide resolved
Versioning_Engine/Compute/VersionKey.cs Outdated Show resolved Hide resolved
Versioning_Engine/Query/VersionKey.cs Outdated Show resolved Hide resolved
Versioning_Engine/Compute/VersionKey.cs Outdated Show resolved Hide resolved
Versioning_Engine/Query/VersionKey.cs Outdated Show resolved Hide resolved
@FraserGreenroyd
Copy link
Contributor

Looks like everything is working as described for me from this code 😄

image

@adecler adecler requested a review from FraserGreenroyd April 23, 2020 09:50
@adecler
Copy link
Member Author

adecler commented Apr 23, 2020

@FraserGreenroyd , I fixed the problems raised in your review

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

Am happy with the changes and am happy with the output (see previous comment).

@kThorsager
Copy link
Contributor

Perhaps intended due to Modify -> Compute not working but if a method is found in both it returns nothing.
image

Worth flagging as that is a planned feature (think this one)

@FraserGreenroyd
Copy link
Contributor

Perhaps intended due to Modify -> Compute not working but if a method is found in both it returns nothing.

I think from the demo @adecler gave this morning, the matching starts from the right and works left, so with those inputs it would be looking for BH.Engine.Geometry.Join which of course doesn't exist (based on my understanding of it).

Maybe a warning to the user to say that the provided thing couldn't be found might be worthwhile? Or if the issue you link is the right one then solving that at a later date.

@kThorsager
Copy link
Contributor

Perhaps intended due to Modify -> Compute not working but if a method is found in both it returns nothing.

I think from the demo @adecler gave this morning, the matching starts from the right and works left, so with those inputs it would be looking for BH.Engine.Geometry.Join which of course doesn't exist (based on my understanding of it).

Maybe a warning to the user to say that the provided thing couldn't be found might be worthwhile? Or if the issue you link is the right one then solving that at a later date.

No, sorry, re-read the description and sounds right (but thought the demo showed using just a part of the namespace)

@FraserGreenroyd
Copy link
Contributor

No, sorry, re-read the description and sounds right (but thought the demo showed using just a part of the namespace)

It does, but it started from the right rather than the left. So I think something like Modify and Join would find the right one (or all the ones that match) because there is *.Modify.Join methods if that makes sense?

@adecler
Copy link
Member Author

adecler commented Apr 24, 2020

A simple way to settle that discussion is to look at the code:

return Engine.Reflection.Query.AllMethodList()
                .Where(x => x.Name == methodName && x.DeclaringType.FullName.EndsWith(declaringType))
                .Select(x => x.VersioningKey())
                .ToList();

I was using EndsWith instead of == so that users wouldn't have to figure out whether to provide just the name of the declaring type or its full name (including namespace). The nice side effect was that yo could even do partial namespaces. I do however regret showing you that since it has been misinterpreted. The input name is declaringType. As far as I know, BH.Engine.Geometry is a namespace, not a declaring type.

@adecler
Copy link
Member Author

adecler commented Apr 24, 2020

/azp run BHoM_Engine.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adecler adecler merged commit 46a5fec into master Apr 24, 2020
@adecler adecler deleted the Post_Build-#245-DecentraliseVersioning branch April 24, 2020 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants