Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Initial contribution core/log.js #268
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
base: master
Are you sure you want to change the base?
Initial contribution core/log.js #268
Changes from 1 commit
995c762
f25bf61
91a89a0
067dac6
45ea7e4
7d5ea41
53cc387
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using an underscore prefix for private vars. @openhab-5iver is this a coding style that we should be using in general?
Also note that these vars are still accessible; if you want to actually make them inaccessible, I'd suggest that you move the definition of these vars from the object block into the containing function body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am aware of that and even did it this way originally. But then I got worried if this would not cause an issue when I instantiate multiple Loggers as I got confused by all these JS context mechanism. So I just moved it into the returned object, well aware that I loose the privacy.
Maybe I should have just taken the time and test it out. I will do actually do this and move them into the function body, if it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be my preference. That's how we do it in Python, so it fit in perfectly IMO!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the vars are defined in the function, they should map 1:1 to the object being returned so you should be fine.
@openhab-5iver I can certainly see the bias for Python here! I agree that striving for alignment is good. I would only caution that there are likely to be areas requiring divergence if you want the JS support to be idiomatic to JS, not Python (although I don't think this is one of them!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truly, it is not bias. I'm attempting to keep some consistency in place to ease the future migration to a Scripting API, which will absorb pretty much all of the helper library functionality. That will take some time though, and development is continuing. I'm hopeful that, with the new development efforts on the JS libs, there will be some collaboration on how to move forward with bringing their functionality to the level of the Python libs.
I have done plenty of development, but my background is from the management side of things. I'm currently the only active maintainer in this repo, and I am very open to collaboration on its technical direction, especially with the JS libs!