HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.

Pull requests & you

LincLinc Detroit Admin

This is a quick overview of pull requests for Vanilla and how the community can help get releases out the door faster.

The very basics:

  • Fork the repository. (You won't be able to push to our repos directly.)
  • Pull request against master branch (FROM the branch on your fork TO our copy's master)
  • Verify you have a CLEAN pull request without extra garbage in the change set.
  • Write a great, release-notes-worthy title for the pull request.
  • Write a well-considered description that details the changes being made & your rationale.
  • Treat GitHub as an extension of the community: be friendly, say hello to new folks, etc.
  • Mention what you've done to test it already, and what others should be looking at in their tests.

Beyond the basics:

Community evaluation of pull requests would be tremendously helpful. I realize you can't merge PRs, but there's a lot of work that happens before we get that far. And, you'll learn a tremendous amount by jumping in.

If you don't have a deep Vanilla understanding, you can look at the superficial: does it meet our coding standards and naming conventions? Is it a decent idea? Is it free of syntax errors? Has the user signed our contributors' agreement?

If you've been around a while and have been building some serious addons, go further: Is this a safe & sane idea? Can you test it in multiple scenarios to see if it causes a regression or unintended consequences? Is this a Vanilla-like thing to do? Is there already a simpler way to accomplish what the author wants without their change? (e.g.: if they say they need a hook to do X, do you know an existing hook or better way to do it?)

When you evaluate a PR, say so in the comments. We're not looking for cheerleader comments like "yay good idea", but rather "I agree this is an issue/good improvement because I've seen X or understand what they're trying to accomplish. I tested this PR by doing exactly W, Y, and Z and these were my results".

The backlog is a bit long right now, but we're gonna get 'er done. :) Keep pullin'!

«1

Comments

  • hgtonighthgtonight ∞ · New Moderator

    If you want to be notified of PRs and have a github account, click the Watch icon and select Watching on the main Vanilla repository here: https://github.com/vanilla/vanilla

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

  • R_JR_J Ex-Fanboy Munich Admin
    edited September 2014

    @Linc said:

    • Features & significant changes: Pull request against stage branch.
    • Hotfixes & tiny, safe changes: Pull request against master branch.

    I only have a 2.1 installation by now. Do the two points above implicit that

    a) I have to install master and stage branch
    b) or are those identical versions with simply two different names
    c) I first have to check if problem I found in 2.1 still prevail in master/stage
    d) I have to fix and test against master/stage

    I have a fix for an issue and normally would have made a Pull Request against the 2.1 file, but it has been changed in 2.2 and so I think the answer to my questions above are a) yes b) ??? c) yes and d) also yes, but could you (or anyone else) please confirm that (and answer b) for me)? I wouldn't like to install something if I didn't have to.

    Edit: found an anwer for b) https://github.com/vanilla/vanilla/compare/stage
    They are not the same, so I need two installations

  • LincLinc Detroit Admin
    edited September 2014

    @R_J That was general Vanilla development procedure. If you have a PR for a 2.1 patch, by all means, pull against 2.1. It's very helpful if you also include notes like "this is already fixed in master" or "this should go to master" or "this is still a problem in master but this won't fix it" - but not required, certainly.

    Stage is always slightly ahead of master; it's where we land new features before they go to master. Master is in an always-ready-to-deploy state. Stage is in a "probably OK but you'll wanna integration test it more first" state.

  • R_JR_J Ex-Fanboy Munich Admin

    Installation already done - both stage and master! :)

    And that was a good thing to do because the issue I was looking at has been already fixed for 2.2 (reported that on GitHub a second before).

  • LincLinc Detroit Admin
    edited September 2014

    Pro tip: I do 1 install each for release branches (2.0 and 2.1), and 1 for master/stage. My installs are directly linked to my git repos so I can toggle as I need to by just checking out a branch. It's extremely rare you would run into a breaking change switching between the two (or one that requires more than utility/update).

    I don't recommend checking out between 2.0/2.1/master on your installs because that will get messy. I have 3 repo copies of Vanilla.

  • LincLinc Detroit Admin

    Also, remember that every fix in master is a fix for the next release :) The more we deal with via master/stage, the better 2.3 will be.

  • chanhchanh OngETC.com - CMS Researcher ✭✭

    This is helpful!
    Thanks

  • peregrineperegrine MVP
    edited September 2014

    Also, remember that every fix in master is a fix for the next release

    can you explain more fully.
    is it something you plan to do in the future.

    e.g. will 2.3 be using upgraded jQuery that master now uses? will ALL bug fixes that have been implemented in master now, be in 2.3

    I may not provide the completed solution you might desire, but I do try to provide honest suggestions to help you solve your issue.

  • JasonBarnabeJasonBarnabe Cynical Salamander ✭✭

    Thanks for the info. It's a bit disheartening to go through the trouble of submitting pull requests only to have them untouched, so this is encouraging.

    Does everything also apply to the addons repository?

  • @JasonBarnabe there is no guarantee that a pull request will get a pass. Not everything belongs in the core an some issue are more important than others.

    grep is your friend.

  • LincLinc Detroit Admin
    edited September 2014

    @JasonBarnabe said:
    Thanks for the info. It's a bit disheartening to go through the trouble of submitting pull requests only to have them untouched, so this is encouraging.

    I fully commiserate, and apologize for the lapse. Obviously not all will make it into core, but we should communicate that ASAP.

    @JasonBarnabe said:
    Does everything also apply to the addons repository?

    It does. I plan to fork Addons to have a 2.3 branch this time as well. We haven't done release branches there in the past, and it's gotten us into trouble.

  • LincLinc Detroit Admin

    @peregrine said:
    can you explain more fully. is it something you plan to do in the future.

    Great question.

    Whenever we start a new release cycle, we copy the current state of master as the seed of the next release. This was less obvious with 2.1 because we forked it so far in the past before it finally made it to release. It took more than a year from when it split from master to when we were "official". That's really bad; 2.3 should be closer to 3 months, I hope.

    The idea is that we have big stuff coming into master constantly that would destabilize our release. So we take a snapshot (fork) and from then on just add smaller patches until we get the official release. master keeps chugging along, never noticing it had a baby. :)

  • LincLinc Detroit Admin

    Excepting 2 I commented on for further assistance, I've caught up all pull requests to 2014.

  • LincLinc Detroit Admin
    edited December 2014

    I made a new badge for pull requests that leave me particularly chuffed: http://vanillaforums.org/badge/high-five

  • LincLinc Detroit Admin

    A new way for super-fast pull requests: https://github.com/blog/1945-quick-pull-requests

  • R_JR_J Ex-Fanboy Munich Admin

    Git is still very hard for me to understand :(
    When I want to make a pull request that affects only one file, I simply do it over the GitHub web interface by directly editing the file. That already creates a pull request.

    In what way is that GitHub Flow different?

  • hgtonighthgtonight ∞ · New Moderator

    @R_J said:
    Git is still very hard for me to understand :(
    When I want to make a pull request that affects only one file, I simply do it over the GitHub web interface by directly editing the file. That already creates a pull request.

    In what way is that GitHub Flow different?

    Looks like it gives you a chance to name the branch

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

  • chanhchanh OngETC.com - CMS Researcher ✭✭

    That is nice addition!
    Thanks for the info.

Sign In or Register to comment.