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

Make `context.issue` and `context.repo` objects #61

Merged
merged 1 commit into from Mar 20, 2019

Conversation

@jclem
Copy link
Contributor

jclem commented Mar 20, 2019

The way that context.issue and context.repo are functions that merge their arguments has been a point of confusion for me quite a bit.

Given that Node supports object spread, and you can achieve the same goal with that, does it make sense for context.issue and context.repo to just be (frozen) objects, instead? Before I read over the readme in much more detail, it's how I expected it to work, and looks a little more idiomatic to me.

const payload = context.issue({body: 'Hello, world!'})

vs

const payload = {...context.issue, body: 'Hello, world!'}
@JasonEtco

This comment has been minimized.

Copy link
Owner

JasonEtco commented Mar 20, 2019

I'm so glad that someone finally said something 😅 this is a remnant of Probot's code (I quite literally copy-pasted), and that decision was made in Probot before the object spread operator was in LTS Node. That's not a good enough reason for it to be here though!

Unfortunately, this is the kind of change that will break almost every action, so it'll need mean a breaking change/new major version to v2 (which should already be done for #41). In any case, still very much in favor.

@jclem

This comment has been minimized.

Copy link
Contributor Author

jclem commented Mar 20, 2019

Sounds good! This was a quick change, so no worries if you close this PR and implement it later!

@JasonEtco

This comment has been minimized.

Copy link
Owner

JasonEtco commented Mar 20, 2019

I think this is a good enough reason to introduce a new major version, especially because of #41 - I'll get a v2 branch going and update this PR's base 🎉 Thanks @jclem ❤️

@JasonEtco JasonEtco mentioned this pull request Mar 20, 2019
2 of 2 tasks complete
@JasonEtco JasonEtco changed the base branch from master to v2 Mar 20, 2019
@JasonEtco JasonEtco mentioned this pull request Mar 20, 2019
2 of 2 tasks complete
@JasonEtco JasonEtco merged commit 77c19fb into JasonEtco:v2 Mar 20, 2019
3 checks passed
3 checks passed
Travis CI - Pull Request Build Passed
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 07e0eca
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.