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

Reflection_Engine: Add method to detect if an item is part of the curated BHoM #2590

Merged
merged 4 commits into from
Aug 11, 2021

Conversation

adecler
Copy link
Member

@adecler adecler commented Aug 9, 2021

Issues addressed by this PR

Contributes to BHoM/Grasshopper_UI#631

Additional comments

@FraserGreenroyd , I have placed your file containing the list of dlls here: C:\ProgramData\BHoM\Settings\IncludedDlls.txt

@adecler adecler added the type:feature New capability or enhancement label Aug 9, 2021
@adecler adecler self-assigned this Aug 9, 2021
@FraserGreenroyd FraserGreenroyd changed the title Add method to detect if an item is part of the curated BHoM Reflection_Engine: Add method to detect if an item is part of the curated BHoM Aug 10, 2021
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.

One minor description update that I'm going to commit as part of this review but I have to request changes to achieve that.

Reflection_Engine/Query/IsPrototype.cs Outdated Show resolved Hide resolved
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.

I have tested this with a couple of components with and without the included DLLs file.

I'm happy the functionality here is meeting the needs.

However, I am currently holding off approving, simply because if merged (along with the Grasshopper PR), and the installer is not updated to dispatch an included DLLs file to users, all components of the BHoM in the next alpha after merge will be marked as prototype.

I'm happy to approve once the installer is updated (or sooner if there is argument to merge sooner), as the functionality works well.

@adecler I updated one description, worth double checking you're happy with it as well before merge 😄

@adecler
Copy link
Member Author

adecler commented Aug 11, 2021

Thanks @FraserGreenroyd ,

Having all components tagged as prototypes if the IncludedDll file is missing would be less than ideal. So have have tweaked to code slightly to account for that scenario.

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.

Have reviewed again with the changes made for the lack of file option and the colour changes by @IsakNaslundBh and am still happy with functionality. With the PR on the installer also ready to go, I am now approving these in line ready to be merged for tomorrows alpha 😄

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 11, 2021

@FraserGreenroyd to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check copyright-compliance
@BHoMBot check dataset-compliance
@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 11, 2021

@FraserGreenroyd to confirm, the following checks are now queued:

  • copyright-compliance
  • dataset-compliance
  • ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 11, 2021

FAO: @FraserGreenroyd
@FraserGreenroyd is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is null-handling.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a CI/CD instruction. I am authorising dispensation to be granted on check ref. 3299909402

@FraserGreenroyd
Copy link
Contributor

@BHoMBot this is a CI/CD instruction. I am authorising dispensation to be granted on check ref. 3299909402

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 11, 2021

@FraserGreenroyd I have now provided a passing check on reference 3299909402 as requested.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 11, 2021

@FraserGreenroyd to confirm, the following checks are now queued:

  • ready-to-merge

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 11, 2021

@FraserGreenroyd to confirm, the following checks are now queued:

  • ready-to-merge

@FraserGreenroyd FraserGreenroyd merged commit ea629aa into master Aug 11, 2021
@FraserGreenroyd FraserGreenroyd deleted the Grasshopper_Toolkit-#631-PrototypeLabel branch August 11, 2021 13:37
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.

2 participants