search

googlearchive / caja

Caja is a tool for safely embedding third party HTML, CSS and JavaScript in your website.
Apache License 2.0
1.13k stars 113 forks source link

Fix issue with arguments.caller and whitelist Array.from #2011

Closed metaweta closed 7 years ago

erights commented 7 years ago

@kpreid , I know you have a different process in mind, but would you mind if just I hit "merge" on this pull request? If so, could you guide Mike through your process? Thanks.

kpreid commented 7 years ago

@erights Two things:

  1. Never merge-commit, only rebase, unless merging is necessary for other reasons. (GitHub recently added support for rebasing and the ability to turn off the merge option on PRs, so I just did that, so that shouldn't be a problem. GitHub calls the correct operation “rebase and merge”.)

  2. The policy we have so far kept from SVN was that the commit description should link to the code review so as to have the reviewer's comments findable in the permanent record. Unfortunately that implies the silly conclusion that one should create a PR and then edit the commits to include the URL, or use Rietveld.

    Given the overall situation, I'm okay with skipping Rietveld and skipping the review link, at least for small changes like this. But that would be a change in project policy. Fellow project member, what do you think of this change in policy, considering the original goals?

jasvir commented 7 years ago

Being able to find codereview comments for a given commit is super helpful. We setup the process initially to include a link to the codereview because we were using a code review tool that was separate from the code repo. If we switching to doing reviews on a github PR itself, then it meets the need to be able to find the review from a commit without having to artificially add a link.

kpreid commented 7 years ago

@jasvir How does that work? I looked at another project's commit-originated-from-a-pull-request and I didn't see any link to the pull request except for the one explicitly mentioned in the description of the merge commit, and we're not going to do merge commits.

erights commented 7 years ago

Just double checking before I press the button:

May I proceed to hit the button above clearly label "Rebase and merge"?

kpreid commented 7 years ago

@erights : If you agree that it is OK not to have a link to the review in the permanent record, then yes, otherwise we need to do something else.

erights commented 7 years ago

For this CL it is ok.

kpreid commented 7 years ago

Hurrah!

Slayer95 commented 7 years ago

@kpreid , the PR is automatically linked from the commit history

kpreid commented 7 years ago

@Slayer95 Thanks for pointing that out! (That sure is some tiny gray text.)