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

Completely refactor Revit Requests #364

Closed
alelom opened this issue Sep 6, 2019 · 9 comments · Fixed by #520
Closed

Completely refactor Revit Requests #364

alelom opened this issue Sep 6, 2019 · 9 comments · Fixed by #520
Assignees
Labels
severity:critical No workaround exists. Essential to continue size:L Measured in days type:compliance Non-conforming to code guidelines

Comments

@alelom
Copy link
Member

alelom commented Sep 6, 2019

Broken rules:

As discussed here: BHoM/BHoM_UI#121 (comment)

Requests in Revit are improperly implemented and cause compliance issues. Consequently the menu is not structured as it should and Requests are difficult to find.

Suggestions to restore compliance:

  1. All Revit requests should not be of type FilterRequest. That is a misuse of the FilterRequest type. They must be their own implementation of IRequest. So, for example, the SelectionFilterRequest:

    public static FilterRequest SelectionFilterRequest()
    {
    FilterRequest aFilterRequest = new FilterRequest();
    aFilterRequest.Type = typeof(BHoMObject);
    aFilterRequest.Equalities[Convert.FilterRequest.RequestType] = RequestType.Selection;
    aFilterRequest.Equalities[Convert.FilterRequest.IncludeSelected] = true;
    return aFilterRequest;
    }

    must be rewritten as composed by two parts:

    • a new BH.oM.Revit -> SelectionRequest class
    • a BH.Engine.Revit.Create method that returns a new SelectionRequest, much like the code above but without the use of FilterRequest.
  2. The previous point must be done for all Revit Requests until no FilterRequest is left.

Note that all Requests must live under the Revit namespace as specified in point 1, unlike the current location under the "Adapters.Revit" namespace:


the "Adapters" part is wrong as we are in an Engine part of the code. You can't have both Engine and Adapter in the namespace, only Engine is correct in this case.

@adecler @al-fisher please correct me if wrong / see if I missed anything

@alelom alelom added type:compliance Non-conforming to code guidelines size:M Measured in hours severity:medium Slows progress, but workaround is possible labels Sep 6, 2019
@alelom alelom added this to the BHoM 2.5 β MVP milestone Sep 6, 2019
@adecler
Copy link
Member

adecler commented Sep 7, 2019

Thanks @alelom ,

I would just add for clarity that everything that was stored inside the Equality dictionary now needs to be stored in its own property. This means you will likely have to create multiple IRequest classes. This also provides a better experience for the end user since the Request components will clearly indicate what inputs are expected.

@ZiolkowskiJakub ZiolkowskiJakub added severity:critical No workaround exists. Essential to continue and removed severity:medium Slows progress, but workaround is possible labels Oct 2, 2019
@pawelbaran pawelbaran removed their assignment Oct 2, 2019
@ZiolkowskiJakub
Copy link
Member

ZiolkowskiJakub commented Oct 15, 2019

Hi @alelom,

I am running couple of tests on implementation of IRequest and I wonder if you could answer one of my questions:

Revt_Toolkit implements RevitUIAdapter which inherits from BHoMAdapter. Currently, the overriden Read method (public virtual IEnumerable<IBHoMObject> Read(FilterRequest request)) is used to pull objects via Adapter.

As you can see it only accepts FilterRequest on input therefore I cannot use this method with IRequest implementation. The question is:

Which method should be used to pull object via Adapter after implementation of IRequest? Should it be 'Pull' method (public virtual IEnumerable<object> Pull(IRequest request, Dictionary<string, object> config = null))? Is it fully implemented in BHoM?

@alelom
Copy link
Member Author

alelom commented Oct 15, 2019

Instead of overriding Read(FilterRequest request), you should implement another Read(IRequest request). Let's have a skype for further clarification

@ZiolkowskiJakub
Copy link
Member

This issue has been parked after discussion with @alelom

These BHoM Adapter issues have to be solved before:
BHoM_Adapter Issue 149
BHoM_Adapter Issue 148

@ZiolkowskiJakub ZiolkowskiJakub removed the status:parked PR on hold, requiring further discussion or planning label Oct 15, 2019
@ZiolkowskiJakub
Copy link
Member

Workaround suggested by @alelom :

Override the Pull and get it to call the Read(IRequest) method

@pawelbaran
Copy link
Member

@alelom @IsakNaslundBh would you be fine with taking over this issue?

@alelom
Copy link
Member Author

alelom commented Nov 11, 2019

Hey @pawelbaran, I remember I had a go in trying to make things compliant here but took me a while just to get oriented. I will have another go on this as part of Adapter Refactoring 04, but I will definitely need some help, especially on the testing side.

@FraserGreenroyd
Copy link
Contributor

@alelom you'll have all the support we can muster for testing, I think @pawelbaran is referring to writing the code to resolve the issue primarily 😄

@pawelbaran
Copy link
Member

@FraserGreenroyd is perfectly right - I am happy to support in testing and review, but would prefer the code to be written by someone with better understanding of the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:critical No workaround exists. Essential to continue size:L Measured in days type:compliance Non-conforming to code guidelines
Projects
None yet
5 participants