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

Detach stamp.static() and stamp.compose() from this #108

Merged
merged 2 commits into from Jun 13, 2015
Merged

Conversation

koresar
Copy link
Member

@koresar koresar commented Jun 13, 2015

Use closure variable instead of this. Stamp function should not depend on bound context. This improves code stability. Stamp behavior gets more predictable.

@unstoppablecarl
Copy link
Contributor

👍 agreed

@koresar
Copy link
Member Author

koresar commented Jun 13, 2015

@unstoppablecarl You have the powers to merge it. :)

@unstoppablecarl
Copy link
Contributor

We should decide on an approval process.

@koresar
Copy link
Member Author

koresar commented Jun 13, 2015

Processes are good in big organisations. In open source projects processes are evil. :) They delay productivity.

If you have any process proposal - go ahead. We'd love to listen.

koresar added a commit that referenced this pull request Jun 13, 2015
Detach `stamp.static()` and `stamp.compose()` from `this`

Merging this as a minor change. Tests are ok.
@koresar koresar merged commit 439ec0d into master Jun 13, 2015
@koresar koresar deleted the detach-form-this branch June 13, 2015 15:37
@unstoppablecarl
Copy link
Contributor

Maybe something like 3 people should review before merging?

@koresar
Copy link
Member Author

koresar commented Jun 13, 2015

Sound okay. But each PR would take about 3 to 5 days to review because there are 4 collaborators here. :)

Let's try be less burocratic. Small safe changes are fine to merge by a single person I think. If a change is significant then let's wait for few people to take a look. Just like I did with the #96

Reasonable?

@troutowicz
Copy link
Member

We can do something similar to what FB does for React. Make a tag GH Review: review-needed and one for when reviewers feel it is good to merge GH Review: accepted.

@koresar
Copy link
Member Author

koresar commented Jun 13, 2015

Good idea. Are we big enough to introduce this little complexity in sake of quality?

@troutowicz
Copy link
Member

I would say that once you have more than two collaborators on a project, a simple change like this would make PR reviews easier to manage. First person to see the PR adds GH Review: review-needed and could cc someone else to handle the review. Or they could review right away by themselves. Once the reviewer thinks things check out they can add the GH Review: accepted tag and cc someone to handle the merge, or possibly handle the merge themselves.

cc @unstoppablecarl @ericelliott

@koresar
Copy link
Member Author

koresar commented Jun 13, 2015

Sounds okay to me. We can try.

@ericelliott
Copy link
Contributor

For small, non-breaking changes and minor bug fixes, the first reviewer should feel free to merge as long as the build is passing and they've looked over the code and feel confident.

For larger changes such as API additions or breaking changes where broader discussion is warranted, (like #96), I agree that other core contributors should have a chance to weigh in before it hits master. =)

In those cases, we should definitely leave it open for discussion for a few days and give other contributors a chance to weigh in.

@koresar koresar modified the milestone: 2.1 Jun 28, 2015
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 this pull request may close these issues.

None yet

4 participants