BHoM / BHoM_Adapter

GNU Lesser General Public License v3.0
7 stars 5 forks source link

All Adapter oM and Engine namespaces should be External #210

Closed al-fisher closed 4 years ago

al-fisher commented 4 years ago

As per @alelom's post here

All "external" (meaning, non BHoM) software Schemas and Method must follow the following convention:

  • oM:
    • BH.oM.External.Revit
    • BH.oM.External.GSA
    • BH.oM.External.Dynamo
  • Engine:
    • BH.Engine.External.Revit
    • BH.Engine.External.GSA

Raising in this Repo, but naturally this is a compliance across all adapter toolkits . Could not find another issue closing this out globally.

Suggest this is important to do before end of RC milestone as this particular compliance has a direct impact on UI and therefore user experience - especially now as code base is becoming quite expansive.

The result of inconsistency of adapter namespaces is that component menus (for any Create, Query, Compute etc.) have a mixture of Software methods appearing in a variety of places - at top level (alongside disciplines), as well as inside an Adapter and/or Adapters sub-menu.

Have raised a comment on previous Test_Toolkit issue, for turning into a formal Compliance test. Would not wait for that to completed though as this is a simple check and change to be made.

With versioning toolkit now - a change of namespace is effectively a simple one liner? (@adecler)

Edit: Instructions and Toolkit checklist: https://github.com/BHoM/BHoM_Adapter/issues/213#issuecomment-597186509

@IsakNaslundBh @FraserGreenroyd @adecler @rwemay FYI

alelom commented 4 years ago

@alelom I would suggest you create a check list of all adapters (especially in installers) to be ticked off? How does that sound?

Sure. I will create a check list of all adapters similar to the last one used for the Refactoring level 04. I will action this week.

Suggest this is important to do before end of RC milestone as this particular compliance has a direct impact on UI and therefore user experience

I will ask people to meet this deadline. It should rather quick and without big repercussions, unlike the Refactoring level 04, so the 20th March deadline should be achievable. It means mid-next sprint, so this will be a topic that I will cover during the sprint opening.

IsakNaslundBh commented 4 years ago

Just some headsup, this will require modifications of the Versioning_Toolkit for many cases. Just to make sure we wont get massive clashes in that toolkit when everyone tries to update all at once. Sort of a more opposite coordination will be needed for this than the one we usually do for the merge parties. Only one toolkit allowed at the time!

alelom commented 4 years ago

Sort of a more opposite coordination will be needed for this than the one we usually do for the merge parties. Only one toolkit allowed at the time!

Better think this through then. I'm busy until later today but I'll then try to reach you for a chat.

FraserGreenroyd commented 4 years ago

Better think this through then. I'm busy until later today but I'll then try to reach you for a chat.

Might be good to include @al-fisher and myself in that chat if we're available - this will require good coordination to meet 20th Deadline for sure...

@IsakNaslundBh maybe it's worth raising one PR (as draft) that everyone contributes to instead? A large multi-authored commit which @adecler then approves (even if he has to add to it as well)?

IsakNaslundBh commented 4 years ago

@IsakNaslundBh maybe it's worth raising one PR (as draft) that everyone contributes to instead? A large multi-authored commit which @adecler then approves (even if he has to add to it as well)?

For sure would be one solution if easier. Do not think we have a namespace upgrader as of yet. Would be good to get that in before we start, as this will be major namespace tweaks of the full project, rather than class per class and method by method, as I can imagine the other option will leave us with a quite significant amount of lines in the code.

If we want to add this functionality in, maybe could be done in the same PR, but not sure if that is a good idea or not @adecler

FraserGreenroyd commented 4 years ago

If we need updates to versioning TK or Engine then for sure we need to align the plan with @adecler and the sprint actions. Maybe best to have a chat very early tomorrow morning with @adecler as well then @alelom and see what the score is?

al-fisher commented 4 years ago

Coordination chat to plan sounds perfect - free later, or a chat early tomorrow with @adecler sounds good

alelom commented 4 years ago

I'm now back - was kind of hoping to have the chat now, but I can see everyone's busy or away. As I communicated earlier individually to each of you, early tomorrow me and @adecler already have scheduled a call for the API docs. I'd like to keep that chat so we can close that item. Can we have the chat all together on this topic early Wednesday instead?

FraserGreenroyd commented 4 years ago

What time is your call with @adecler tomorrow scheduled to finish?

I fear the later we discuss it, the later we get to making a decision, the shorter the implementation time if we decide we're doing it today. If we can't discuss today, and we can't discuss with @adecler in the morning, then I propose we outline the questions we have for @adecler and he can answer them here instead? Not as great as a vocal discussion with him but might be enough for us to make the decisions we need to make tomorrow to move this forward?

adecler commented 4 years ago

With versioning toolkit now - a change of namespace is effectively a simple one liner? (@adecler)

Yes, one line per namespace (not per class) is enough. So you can effectively do something like BH.oM.Revit --> BH.oM.External.Revit

One thing that might be worth mentioning since you are starting to be more active in the versioning toolkit is that I intent to modify the way the ToNewMethod dictionary stores the new method. If they was stored as a string instead of a MethodBase, that would reduce massively the number of dlls to reference. Basically only the deprecated objects with complex conversion would need dll references. It shouldn't affect your plan since it is purely an internal change but a good one to keep in mind.

I propose we outline the questions we have for @adecler and he can answer them here instead?

Sounds good to me. If you have the meeting between 10am and 11am UK time, send me an invite anyway and I'll try to at least listen in. No promise though.

epignatelli commented 4 years ago

Is there a codebase already that I can use to test the effects of removing the external keyword?

IsakNaslundBh commented 4 years ago

Is there a codebase already that I can use to test the effects of removing the external keyword?

Could use this one https://github.com/BHoM/MidasCivil_Toolkit/pull/203

alelom commented 4 years ago

Refer to https://github.com/BHoM/BHoM_Adapter/issues/213#issuecomment-627929472 for continuation of the conversation