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

Rename private functions to start with underscore #33

Closed
ialokim opened this issue Aug 25, 2020 · 2 comments · Fixed by #37
Closed

Rename private functions to start with underscore #33

ialokim opened this issue Aug 25, 2020 · 2 comments · Fixed by #37

Comments

@ialokim
Copy link
Contributor

ialokim commented Aug 25, 2020

My recent work on the gate and wire models introduced some additional methods, some of them are meant to be used from outside of the library (part of the API) while others are just internal helper functions. To distinguish them easier, I would propose to follow naming conventions and rename internal methods to start with the prefix _.

Also, from an esthetic point of view, I would propose to unify method declaration syntax used throughout this project and decide whether to use name: function(argument) {} or name(argument) {}. I don't know if there is a "better" way to declare methods, but I think it would make sense to stick to one of them.

If you were okay with these changes, I would be happy to realize them.

@tilk
Copy link
Owner

tilk commented Aug 25, 2020

I'm all for clearly marking internals, and using an underscore prefix is a popular convention. I think it's worthwhile to do it, and this feels like a prerequisite before 1.0.0 release (I'm still using 0. release numbers, as I feel the API is still in flux and it would be a PITA to use semver.)

On declaration syntax, I prefer the "new" syntax (without the "function" keyword), as it's less typing and closer to how you write in e.g. TypeScript. (By the way, core DigitalJS is pure JS instead of TypeScript because of how Backbone works poorly with it.)

@ialokim
Copy link
Contributor Author

ialokim commented Sep 25, 2020

I'm going to refactor accordingly after #31 has been discussed and (hopefully) merged.

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

Successfully merging a pull request may close this issue.

2 participants