-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix: use es6 for cloudevents.js #73
Conversation
75928d6
to
c0c5840
Compare
The Travis failure is not due to changes in this PR. |
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.
This is an incremental improvement but dances around the goal of idiomatic code style. For instance it doesn’t take any steps towards addressing #65. I don’t see harm in landing this but I think some of the larger architectural patterns still need to be dealt with. Moving towards idiomatic code, direct object creation, and immutable instances requires eliminating the setter functions and using class get functions. Personally I think it would be worth exploring that approach.
Yes, I'd like to get there too, but I don't want to change the whole codebase by renaming methods at this point. I'd like to merge this incremental improvement and make small PRs. Right now I can't even make simple changes due to Travis configs and DCO checks which makes things painful. |
@grant you should rebase from |
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
c0c5840
to
4a98edc
Compare
…rant_simplify_cloudeventsjs
I think the PR Quality Review tool is actually wrong here. It's saying an object is a block. |
@grant I agree - the Codacy rule seems broken. |
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.
LGTM
No description provided.