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

fix: resolve invalid conflict resolution #11077 for v6.5 #11356

Merged
merged 1 commit into from Jun 27, 2018
Merged

fix: resolve invalid conflict resolution #11077 for v6.5 #11356

merged 1 commit into from Jun 27, 2018

Conversation

DanielRuf
Copy link
Contributor

Description

Wrong local variable _this was used.

Motivation and Context

Fix the wrong local variable, should be this or e.target.

Screenshots (if appropriate):

Types of changes

  • Documentation
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing
    functionality to change)

Checklist (all required):

  • I have read and follow the CONTRIBUTING document.
  • There are no other pull request similar to this one.
  • The pull request title is descriptive.
  • The template is fully and correctly filled.
  • The pull request targets the right branch (develop or support/*).
  • My commits are correctly titled and contain all relevant information.
  • My code follows the code style of this project.
  • I have updated the documentation accordingly to my changes (if relevant).
  • I have added tests to cover my changes (if relevant).
  • All new and existing tests passed.

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

This comes from a merge conflict that was incorrectly resolved. This issue was already resolved in #11076.

Take a look at the difference between:

The current PR is nice but you may rename it (and the commit too) to indicate that this is more a conflict resolution that a bug fix.

Something like:

fix: resolve invalid conflict resolution #11077 for v6.5

What do you think ?

@DanielRuf
Copy link
Contributor Author

Because this is wrong in 6.5.0 RC1.

@DanielRuf
Copy link
Contributor Author

In general it is a bugfix. Your proposed commit message is not a clear and precise one as it does not describe the change imho.

@ncoden
Copy link
Contributor

ncoden commented Jun 27, 2018

The commit title I proposed describe the purpose of the commit. We are not fixing a bug we just discovered in v6.5, we're re-applying a fix that was removed in a conflict resolution.

The code change and the reasons for it are kept in the same place: the pull request we want to "re-import" #11077.

@DanielRuf
Copy link
Contributor Author

Something like:

fix: resolve invalid conflict resolution #11077 for v6.5

What do you think ?

Done.

@DanielRuf DanielRuf changed the title fix: use local this, fixes #11355 fix: resolve invalid conflict resolution #11077 for v6.5 Jun 27, 2018
@ncoden
Copy link
Contributor

ncoden commented Jun 27, 2018

Nice. Thank you 👍

@ncoden ncoden added this to the 6.5.0 milestone Jun 27, 2018
@ncoden ncoden merged commit e2571bb into foundation:support/6.5 Jun 27, 2018
@DanielRuf DanielRuf deleted the fix/use-local-this branch June 27, 2018 20:31
@DaSchTour
Copy link
Contributor

This would be more readable if lambdas and e.target would be used. This would allow to eliminate the _this;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants