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

Convert method naming convention #222

Open
peterjamesnugent opened this issue Feb 13, 2020 · 2 comments
Open

Convert method naming convention #222

peterjamesnugent opened this issue Feb 13, 2020 · 2 comments
Assignees
Labels
severity:low Doesn't stop/slow current workflow size:S Measured in minutes type:compliance Non-conforming to code guidelines

Comments

@peterjamesnugent
Copy link
Member

Broken rules:

Current naming convention is ToBar, ToPoint, ToBarDistributedLoad etc.

Suggestions to restore compliance:

Change method names for Convert to FromLusas

We ran in to this problem before, because Lusas stores all loads under a single class IFLoading, therefore all Convert methods for loads would have the same input parameters leading to compiling errors. Therefore, we broke all Convert methods down to ToBarPointLoad, ToAreaUniformlyDistributed etc.

So instead of using FromLusas we could be specific:

  • FromLusasPoint
  • FromLusasLine

Using FromLusas[Object type used in Lusas LPI]

Would this be preferable @FraserGreenroyd?

@peterjamesnugent peterjamesnugent added type:compliance Non-conforming to code guidelines severity:low Doesn't stop/slow current workflow size:S Measured in minutes labels Feb 13, 2020
@FraserGreenroyd
Copy link
Contributor

@peterjamesnugent I can't believe I never responded to this, I'm hoping we spoke offline about it cause otherwise this is bad of me and I apologise!

Based on our documentation now, which is perhaps more up to date than it was in 2020, it appears that ToBar and FromBar are acceptable without needing to be ToLusas or FromLusas.

However, it may be appropriate to move the methods up to the Adapter, rather than keeping them in the engine if they are only for Lusas specific workflows.

I think the original intent behind the compliance issue was that ToBHoM and FromBHoM weren't acceptable but ToLusas/FromLusas would be as it made it clear what the to/from was involving (the BHoM part being explicit by the fact the plugin is the BHoM plugin).

So to close this issue out, I think moving the converts up to Adapter but keeping their names would be sufficient.

One for @KalleEdstroem perhaps as a nice easy task following his mammoth work on the new Lusas version? 😄

@KalleEdstroem
Copy link
Contributor

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:low Doesn't stop/slow current workflow size:S Measured in minutes type:compliance Non-conforming to code guidelines
Projects
None yet
Development

No branches or pull requests

3 participants