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

Refactor backend: dataclass static function naming: from vs createFrom #6241

Closed
fm3 opened this issue May 26, 2022 · 3 comments · Fixed by #6360
Closed

Refactor backend: dataclass static function naming: from vs createFrom #6241

fm3 opened this issue May 26, 2022 · 3 comments · Fixed by #6360

Comments

@fm3
Copy link
Member

fm3 commented May 26, 2022

Some of our dataClasses have static methods from(x), some have createFrom(x), some have parse(x).
Also, if there are multiple sources a class instance can be created from, the overloading is sometimes done with actual overloading (all named the same) and sometimes the names differ (fromArray, fromMagLiteral).

I’d say we should define a unified convention and re-name functions we come across accordingly.

Personally, I’d vote to use the last mentioned variant, i.e. overloading explicitly in names, and no “create” – The Vec3Int class already uses this convention.

@leowe @jstriebel any preferences?

@leowe
Copy link
Contributor

leowe commented May 26, 2022

I don't have strong preferences either way, discoverability in an IDE is probably better with the different names.

Would the standard static method then be from(x) and not createFrom(x)?

@fm3
Copy link
Member Author

fm3 commented Jun 2, 2022

Thanks for your input :) Then I’ll set the new convention as I suggested above. Yes, the create prefix should be removed. Ideally it should say fromSomething(x).

I already started propagating this convention in #6249

Note that this is only about dataclass static methods. More complex creation operations like taskCreationService.createTracingsFromBaseAnnotations(…) should still have the create prefix to indicate what is happening.

@jstriebel
Copy link
Contributor

Sorry, I missed to answer here. I fully agree, using from() and fromThisThing() sounds coherent and seems to be a more clear change 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants