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

Remove Adapters namespace in the Engine classes #385

Closed
ZiolkowskiJakub opened this issue Oct 1, 2019 · 14 comments
Closed

Remove Adapters namespace in the Engine classes #385

ZiolkowskiJakub opened this issue Oct 1, 2019 · 14 comments
Assignees
Labels
severity:critical No workaround exists. Essential to continue type:compliance Non-conforming to code guidelines

Comments

@ZiolkowskiJakub
Copy link
Member

ZiolkowskiJakub commented Oct 1, 2019

Broken rules:

As mentioned in #364 by @alelom the 'Adapters' namespace shall not exists in the Engine classes.

Suggestions to restore compliance:

Remove Adapters namespace from Engine classes.

Also, in the Adapter project, make sure the namespace should be Adapter and not Adapters.
Adapters with the "s" should not exist in BHoM.

@ZiolkowskiJakub ZiolkowskiJakub added the type:compliance Non-conforming to code guidelines label Oct 1, 2019
@ZiolkowskiJakub
Copy link
Member Author

ZiolkowskiJakub commented Oct 1, 2019

@al-fisher, @IsakNaslundBh, @alelom if we remove 'Adapters' from namespace of the Engine and oM methods it will cause backward compatibility issues... Do you have any suggestions how to carry out this change? Using 'Deprecated' attribute will double amount of code and will be quite long process to go.

@alelom alelom changed the title Remove Adapters namespace Remove Adapters namespace in the Engine methods Oct 1, 2019
@al-fisher
Copy link
Member

Thanks @ZiolkowskiJakub - agreed the process to carry out these changes to effectively deprecate does need consideration and some planning before we launch in. 👍
Looping in @adecler as some technical and process improvements to backwards compatibility and depreciation are being planned as shown yesterday.
@adecler - following our chats today, can you put a plan in place to communicate what is coming and when? - Thanks!

@ZiolkowskiJakub
Copy link
Member Author

ZiolkowskiJakub commented Oct 1, 2019

@alelom just to confirm: do you want to remove it from Engine only? What about the rest?

Currently we have:
BH.oM.Adapters.Revit
BH.Engine.Adapters.Revit

@alelom alelom changed the title Remove Adapters namespace in the Engine methods Remove Adapters namespace in the Engine and oM classes Oct 1, 2019
@alelom alelom changed the title Remove Adapters namespace in the Engine and oM classes Remove Adapters namespace in the Engine and oM classes. Correct Adapters to Adapter. Oct 1, 2019
@ZiolkowskiJakub
Copy link
Member Author

Hi @alelom,

BH.Adapter.Revit is correct for Revit_Adapter..

Adapters for BH.oM.Adapters.Revit and BH.Engine.Adapters.Revit comes from old approach taken from the past

@alelom alelom changed the title Remove Adapters namespace in the Engine and oM classes. Correct Adapters to Adapter. Remove Adapters namespace in the Engine classes Oct 1, 2019
@ZiolkowskiJakub ZiolkowskiJakub added this to the BHoM 3.0 β MVP milestone Oct 2, 2019
@ZiolkowskiJakub ZiolkowskiJakub added the severity:critical No workaround exists. Essential to continue label Oct 2, 2019
@pawelbaran pawelbaran removed their assignment Oct 2, 2019
@pawelbaran
Copy link
Member

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

@pawelbaran
Copy link
Member

Guys, I can tackle this one (the sooner the better I guess). Happy to do the following renaming:

  • BH.oM.Adapters.Revit to BH.oM.Revit
  • BH.Engine.Adapters.Revit to BH.Engine.Revit

How to tackle that with regard to backwards compatibility? @adecler @al-fisher @rwemay

@IsakNaslundBh
Copy link
Contributor

Would hold off on especially the first of this until https://github.com/BuroHappoldEngineering/Operations_Toolkit/issues/100 has been confirmed.

Think we want to group software specific objects to avoid having them mix to much with the rest of the obejcts.

For the second one I guess we might want to add in 'External' as well, but has not really been discussed that much as of yet.

@alelom
Copy link
Member

alelom commented Nov 19, 2019

BH.Engine.External.Revit was indeed the most successful option when we last discussed. I'd go for it, but then I'd officially formalise it and close https://github.com/BuroHappoldEngineering/Operations_Toolkit/issues/100, what do you say @al-fisher ?

@pawelbaran
Copy link
Member

Not being involved in the discussion at all, I am happy to unassign myself from this one 🐒

@pawelbaran pawelbaran removed their assignment Nov 19, 2019
@alelom
Copy link
Member

alelom commented Nov 19, 2019

I'd say go for it.

Guys, I can tackle this one (the sooner the better I guess). Happy to do the following renaming:

  • BH.oM.Adapters.Revit to BH.oM.Revit
  • BH.Engine.Adapters.Revit to BH.Engine.Revit

For the second one, use:
BH.Engine.External.Revit

if we then change mind on the naming of External (say we change it to any other name), it's just a find-replace in the code. But at least in the meantime the namespace would be sorted correctly.

With regards to the backwards compatibility, that's something additional to be checked with @adecler. The priority should be anyway the namespace renaming, because that would tidy-up the Context menu, where Revit is one of the few exceptions that make it untidy because of its non-compliant namespace names.

@al-fisher
Copy link
Member

Great thanks guys.
From https://github.com/BuroHappoldEngineering/Operations_Toolkit/issues/100 looks like all still in agreement.
Let's proceed with External @pawelbaran 👍

@al-fisher
Copy link
Member

How to tackle that with regard to backwards compatibility? @adecler @al-fisher @rwemay

@adecler has specifically implemented the ability to deprecate where there is a change of namespace only. See PR here: BHoM/Versioning_Toolkit#4
Be good for you to use this as a use-case and further testing of this approach @pawelbaran ! Should be good.
Also @adecler is there any further documentation on how to deprecate to link people too? Useful for more guys to help stress test some of the cases you have already ticked off - appreciate there are a few more cases to come for UI etc. Cheers

@adecler
Copy link
Member

adecler commented Nov 20, 2019

See comment in #450 (comment)
Documentation will come next once the process of versioning has been refined a bit. FYI, in the long run, I intend to have code added to the versioning engine automatically for trivial cases through the Visual studio toolkit (leveraging VS refactoring tools) since the manual process is a bit dull and repetitive.

@alelom
Copy link
Member

alelom commented Mar 12, 2020

I'm closing this as it has been replaced by #517.

@alelom alelom closed this as completed Mar 12, 2020
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 type:compliance Non-conforming to code guidelines
Projects
None yet
Development

No branches or pull requests

6 participants