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

Adding support for method upgrade #8

Merged
merged 2 commits into from
Jan 24, 2020

Conversation

adecler
Copy link
Member

@adecler adecler commented Nov 20, 2019

NOTE: Depends on

Issues addressed by this PR

Closes #7

Test files

https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/01_Test%20Scripts/Versioning_Toolkit/Issue7_MethodUpgradeTest_ForAdapters.gh?csf=1&e=Su4xfT

You will need the following PRs to test this file correctly:

Additional comments

  • This has been done within the context of the interface change for the adapters. So the test files focuses on that.
  • Methods are a bit more tricky than objects because it is not about recovering the content the best we can this time. It is about finding a method that does the same thing even if the inputs/outputs are different. This is why I haven't changed the method deserialiser in the serialise engine to automatically return the new version of the method. This time I want it to keep failing so the UI get a chance to use the rest of its information (i.e. old input/output parameters) to upgrade the component correctly (i.e. reorganise the inputs).
  • Notice that we now have full support for upgrading objects, methods, and namespaces/types. The last two are in a single file (something that we might want to change ?) while the first one is in a folder. I have therefore modified the folder name to Object Converter to clarify the relation with the other two files. Happy for ideas on how better organise the whole thing. Keep in mind though that most of this content (at least types and methods) should be filled automatically through refactoring via the VisualStudio_Toolkit in the medium term.
  • I have left the upgrade methods for the adapter in the PR for testing purposes. Those will have to be commented out if merged before the PRs for the level 4 changes on the adapter (more specifically the UI PR)

@adecler adecler added the type:feature New capability or enhancement label Nov 20, 2019
@adecler adecler added this to the BHoM 3.0 β MVP milestone Nov 20, 2019
@adecler adecler self-assigned this Nov 20, 2019
@adecler adecler requested a review from pawelbaran November 20, 2019 08:22
@adecler
Copy link
Member Author

adecler commented Nov 20, 2019

@pawelbaran , I have added you as a reviewer so you can already give your feedback in preparation of BHoM/Revit_Toolkit#450

@FraserGreenroyd
Copy link
Contributor

@alelom I feel you should take a look at this PR if you can as it is focusing on adapter versioning.

I intend to try and look at it today, but I am cautious of the fact I have reviewed the past couple of versioning PRs and would be good to spread the knowledge to others as well, so @IsakNaslundBh or @epignatelli it would be good if you can take a look today as well? 😄

@epignatelli
Copy link
Member

This depends on a draft pr for testing - is this actually to review?

@alelom
Copy link
Member

alelom commented Nov 29, 2019

It seems some files are missing and that's blocking compilation for me:
image

@adecler
Copy link
Member Author

adecler commented Nov 30, 2019

@alelom , sorry about that, it seems that the project file was not pushed properly last time. Now fixed

@epignatelli , yes it is. But we will have to remove the content of the ToNewMethod dictionary (and the dependency of course) before merging (See my last additional comment). I will add the do-not-merge label to reinforce that.

@adecler adecler added the status:do-not-merge For instance, test PR, requires further discussion, or dependant PRs not ready for merge label Nov 30, 2019
@alelom
Copy link
Member

alelom commented Dec 4, 2019

Thanks for that @adecler. I can now compile successfully.

I now aligned both this PR and BHoM/Socket_Toolkit#57 to the latest changes in BHoM/BHoM_Adapter#164.

However, on opening of GH, I get this:

and on the opening of the test script:

Something about my latest changes must've broken it somehow.
The latest changes I've made are rather small, if compared to the refactor I had done up to the point you wrote this -- I can't figure out where the issue is arising.

I'd appreciate if we could have a look at this together. I'm also happy to have a Skype if you think it's quicker.

@adecler
Copy link
Member Author

adecler commented Dec 5, 2019

@alelom , yes, let's try to have a chat when you get to the office (assuming you don't get there too late 😋 )

@alelom
Copy link
Member

alelom commented Dec 5, 2019

The errors at #8 (comment) were caused by an issue in the Engine, now corrected by BHoM/BHoM_Engine#1377

Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

This is really quite amazing. I could open several old scripts of mine without having to replace a single Push or Pull.

Here's the result of some testing.

Tests of scripts written in Rhino 5

The script linked by @adecler works fine.

Most of the other scripts (written in Rhino 5) that I tried upgraded the Adapter components correctly. Here is an example of one of them where it worked.

However, for one of them I get an error:
image
In this same script I get a very weird-looking Pull component 😝
image

Tests of scripts written in Rhino 6

I could not replicate the desired behaviour for scripts written in Rhino6 / GH version >1.

This is an example of a GH script written in Rhino 6 where this fails.

I always get the following error opening the RH6 scripts:

And Adapter components do not deserialise correctly:

Additional note

I only tested and linked scripts written using the exact same version of BHoM for both Rhino 5 and 6 scripts.
I personally produced those scripts last week by installing the beta release available on the website.

@adecler
Copy link
Member Author

adecler commented Dec 6, 2019

Thanks @alelom , it was actually a bug on the Grasshopper side. If you re-sync and recompile that part, it should now work fine for the pull.

Copy link
Contributor

@IsakNaslundBh IsakNaslundBh left a comment

Choose a reason for hiding this comment

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

General comment/question here: Should the method upgrades introduced in this PR be moved to a new BHoMUpgrader31 project before merging?

Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

Now very happy with this PR! I've re-tested using the files already linked here: #8 (review)
and this works marvellously for all Adapter components.
Thanks!

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.

Add support for method upgrade
6 participants