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

Batch cleanup and fix for #440 #464

Merged

Conversation

pawelbaran
Copy link
Member

Issues addressed by this PR

Closes #440

Test files

Not needed.

Changelog

  • all internal methods in BH.UI.Revit.Engine that are possibly useful outside Revit_Toolkit were made public

Additional comments

I made another iteration of the batch cleanup - looking forward to getting a few of these in the near future.

Copy link
Member

@rwemay rwemay left a comment

Choose a reason for hiding this comment

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

Nice bit of initial tidy up. Approving on the basis of:

  • I've scanned through the code and noted that changes are limited to making internal methods public, or formatting of the green header descriptive text;
  • I rebuilt and did a quick test on a script that pulls grids - all works ok;
  • Good to get uniformity on the public methods for convert/modify/query;

Note that the convert methods don't seem to be in the right namespace (and hence don't show up in the UI). I think should be BH.Engine.Adapters.Revit.Query. With making all these methods public we are naturally exposing a lot into the UI but a) the converts aren't visible anyway at the moment b) I don't think they're widely used in UI's and c) we can always suppress these in some UI's as and where necessary/appropriate.

@pawelbaran
Copy link
Member Author

Note that the convert methods don't seem to be in the right namespace (and hence don't show up in the UI). I think should be BH.Engine.Adapters.Revit.Query. With making all these methods public we are naturally exposing a lot into the UI but a) the converts aren't visible anyway at the moment b) I don't think they're widely used in UI's and c) we can always suppress these in some UI's as and where necessary/appropriate.

All convert/modify/query methods that reference Revit API are stored in BH.UI.Revit.Engine on purpose: Revit API cannot be used outside the Revit context, so there is no benefit of exposing them in the UI (they will all spit errors). However, changing the access level from internal to public makes them available for other projects, which might be useful in some cases.

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.

LGTM... All 288 files of it... 😄

@FraserGreenroyd FraserGreenroyd merged commit 7e9d05b into master Nov 20, 2019
@FraserGreenroyd FraserGreenroyd deleted the Revit_Toolkit-Issue440-MakeAllInternalMethodsPublic branch November 20, 2019 08:49
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.

Revit_Toolkit: Make all BH.UI.Revit.Engine internal methods public
3 participants