Skip to content

Squashed/rebased #1373 #1531

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Squashed/rebased #1373 #1531

wants to merge 1 commit into from

Conversation

twolfson
Copy link

As requested by @jzaefferer in #1512, we have taken care of squashing/rebasing #1373 while maintaining appropriate authorship. To make this easier for future persons, these are the steps I took:

# Added upstream remote
git remote add upstream https://github.com/jzaefferer/jquery-validation

# Installed `upstream-pr` branch
git config --add remote.upstream-pr.url $(git config --get remote.upstream.url) && git config --add remote.upstream-pr.fetch +refs/pull/*/head:refs/remotes/upstream/pr/*

# Fetched upstream PRs
git fetch upstream-pr

# Checked out PR in question
git checkout pr/1373

# Checked out branch for squashing
git checkout pr/1373.squash

# Squashed commits into 1 commit
# Use `git log` to find commit where we diverged from `master
git rebase -i {{last commit from master}}
# Select all but head commit as "squash"

# Check out new branch to perform rebasing
git checkout pr/1373.rebased

# Rebase 1 commit on top of `upstream/master`
git rebase -i upstream/master

# Handle merge conflicts
git mergetool -y

# Verify that diff aligns with current GitHub PR
# Should see no changes here
git diff pr/1373.squash -- src/additional/accept.js
# Should align with diff on GitHub
git diff upstream/master

# Complete our rebase
git rebase --continue

# All done, PR opened

/cc @jzaefferer

@twolfson twolfson mentioned this pull request Jul 22, 2015
Closed
@Arkni
Copy link
Member

Arkni commented Oct 24, 2015

@staabm
This should be merged by now.

@staabm
Copy link
Member

staabm commented Nov 24, 2015

@Arkni missed this PR... could you rebase it against master?

@twolfson
Copy link
Author

This PR is a rebase of #1373 and this was opened 4 months ago under the presumption that it would be landed more/less in a timely manner (i.e. without merge conflict). What's the guarantee that this second rebase will be landed in a timely manner?

@staabm
Copy link
Member

staabm commented Nov 25, 2015

@twolfson I am willing to land it and now also have the required permissions to do it :-)

@twolfson
Copy link
Author

Alright, taking a look at rebasing now

@twolfson twolfson force-pushed the pr/1373.rebased branch 5 times, most recently from 1426f2b to 1cc5524 Compare November 25, 2015 20:48
@Arkni
Copy link
Member

Arkni commented Nov 25, 2015

The test is failing due to this check (src/core.js#L643-L647):

if ( element.hasAttribute( "contenteditable" ) ) {
    val = $element.text();
} else {
    val = $element.val();
}

The mocked file input does not have the hasAttribute method. A quick fix could be:

 test/methods.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/methods.js b/test/methods.js
index 0848357..cf29f04 100644
--- a/test/methods.js
+++ b/test/methods.js
@@ -37,7 +37,8 @@ function acceptFileDummyInput( filename, mimeType ) {
                type: "file",
                files: fileList,
                nodeName: "INPUT",
-               value: "/tmp/fake_value"
+               value: "/tmp/fake_value",
+               hasAttribute: function() { return false; }
        };
 }

@twolfson
Copy link
Author

Sorry, got side-tracked on other work. Thanks for the pointer. I will try that out in a bit

@twolfson
Copy link
Author

@Arkni That patch worked great, thanks =)

@staabm This PR should be good to go

staabm
staabm reviewed Nov 25, 2015
View reviewed changes
test/methods.js
test( "file accept - image wildcard", function() {
var input = acceptFileDummyInput( "test.png", "image/png" ),
$form = $( "<form />" ),
proxy = $.proxy( $.validator.methods.accept, new $.validator( {}, $form[ 0 ] ), null, input, "image/*" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this $.proxy come from?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was in the initial PR (#1373) but it's defined via jQuery and essential is ES5's Function.bind:

https://api.jquery.com/jQuery.proxy/

@staabm
Copy link
Member

staabm commented Nov 25, 2015

Will land it tomorrow after beeing back on the pc. Thx so far.

@twolfson
Copy link
Author

Sweet, thanks =)

@staabm staabm closed this in 9962f62 Nov 26, 2015
@staabm
Copy link
Member

staabm commented Nov 26, 2015

@twolfson landed it right now after a few minor changes in the commit message.

thank you!

@twolfson
Copy link
Author

Cool, thanks =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@twolfson @Arkni @staabm @mishal