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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify elementCount custom assertions #898

Merged
merged 2 commits into from
Nov 21, 2017
Merged

Simplify elementCount custom assertions #898

merged 2 commits into from
Nov 21, 2017

Conversation

robwierzbowski
Copy link
Contributor

@robwierzbowski robwierzbowski commented Sep 3, 2017

This PR takes care of three things:

  1. Slightly improves the file comment.
  2. Renames one of two identically named variables that, because of separate scope, are actually pointing to different pieces of memory. The selector in the function passed to the browser is not the same data as the selector in brackets immediately after it.
  3. Calls a callback directly instead of using .call().

Let me know what you think. #2 was especially confusing to me while I was looking at this file for the first time. If you can think of a better argument name, let me know 馃檭.

Standardize capitalization, punctuation
Use common language
Reduce word count
Before this commit the term `selector` was used for two separate arguments in
two separate scopes. Rename the function passed to the browser so the developer
understands that these are two arguments/two scopes/not the same value.
@robwierzbowski
Copy link
Contributor Author

robwierzbowski commented Sep 3, 2017

Side note: I see a lot of templating to add semicolons. I'm converting all of my code to AirBnb style, and I think it might be possible to write the template files so they can be transformed to the user's preferred formatting (Standard or AirBnB) with a post-install lint --fix command.

Have you tried that / any interest? It could be a lot cleaner than all of the if AirBnB then insert ; statements. I'd be interested in writing a PR.

@LinusBorg
Copy link
Contributor

tbh I'm not super familiar with Nightwatch, but that change for the callback means that this is now undefined in the callback, which the original code probably intentionally avoided.

@robwierzbowski
Copy link
Contributor Author

From what I read in Nightwatch docs, the this binding isn't necessary in the callback. I looked through the history and there aren't any clues to why it was added. I've tested and everything seems to work as expected.

I'm most interested in making the arguments more clear (c3a2181), so if you'd like to keep the bound callback I can remove the commit that changes it.

@LinusBorg
Copy link
Contributor

@vuejs-templates/collaborators Can anybody with more nightwatch experience chime in wether it's imortant to bind this here?

@yyx990803
Copy link
Contributor

this will point to the browser instance, I believe it's a convention for most of the callbacks in nightwatch. I'd preserve it for the sake of compatibility.

@LinusBorg
Copy link
Contributor

@robwierzbowski There's your answer :)

@robwierzbowski
Copy link
Contributor Author

OK, commit removed. Very small PR now :)

@yyx990803 yyx990803 merged commit 5b138f3 into vuejs-templates:master Nov 21, 2017
Quramy pushed a commit to Quramy/webpack that referenced this pull request Nov 22, 2017
* Improve custom Nightwatch assertion comment readability

Standardize capitalization, punctuation
Use common language
Reduce word count

* Clarify elementCount arguments

Before this commit the term `selector` was used for two separate arguments in
two separate scopes. Rename the function passed to the browser so the developer
understands that these are two arguments/two scopes/not the same value.
Quramy pushed a commit to Quramy/webpack that referenced this pull request Nov 22, 2017
* Improve custom Nightwatch assertion comment readability

Standardize capitalization, punctuation
Use common language
Reduce word count

* Clarify elementCount arguments

Before this commit the term `selector` was used for two separate arguments in
two separate scopes. Rename the function passed to the browser so the developer
understands that these are two arguments/two scopes/not the same value.
c0defre4k pushed a commit to neonblack-at/webpack that referenced this pull request Nov 30, 2017
* vuejs-templates/master:
  bump v1.2.4
  Simplify elementCount custom assertions (vuejs-templates#898)
  revert vuejs-templates#996 chunk names to minify prodution JS (vuejs-templates#1086)
  fix missing comma
  Batman!
  add missing cacheBusting option
  make jest ignore the e2e test folder because its likely to contain files matching the jest's filename pattern
  Improved Jest config (vuejs-templates#1074)
  Fix # 1070 When index.html has a positional variable, the error can be printed correctly (vuejs-templates#1071)
  [enhancement]Remove useless files and performance enhancement in dev server. (vuejs-templates#1072)
shenron pushed a commit to shenron/webpack that referenced this pull request Mar 20, 2018
* Improve custom Nightwatch assertion comment readability

Standardize capitalization, punctuation
Use common language
Reduce word count

* Clarify elementCount arguments

Before this commit the term `selector` was used for two separate arguments in
two separate scopes. Rename the function passed to the browser so the developer
understands that these are two arguments/two scopes/not the same value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants