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

[#1102] Update Excalibur to follow style guidelines #1118

Merged
merged 4 commits into from Apr 14, 2019

Conversation

Projects
None yet
3 participants
@DavidLi119
Copy link
Contributor

commented Apr 13, 2019

Changed var to const and let where applicable. Left var in for loops alone.
Works on Issue #1102
Files changed: src/spec/ActionSpec.ts
Notes: VSCode has problems finding module ../../build/dist/excalibur requesting solution please?
Unsure whether to change var on lines 597 and 618 to let or const, currently changed to let if it is fine as it is currently, please let me know thanks.
===馃搵 PR Checklist 馃搵===

  • 馃搶 issue exists in github for these changes
  • 馃敩 existing tests still pass
  • 馃檲 code conforms to the style guide
  • 馃搻 new tests written and passing / old tests updated with new scenario(s)
  • 馃搫 changelog entry added (or not needed)

==================

DavidLi119 added some commits Apr 13, 2019

Update Excalubur to follow style guidelines
Changed `var` to `const` and `let` where applicable. Left `var` in `for` loops alone

@eonarheim eonarheim changed the title Update Excalubur to follow style guidelines [#1102] Update Excalibur to follow style guidelines Apr 13, 2019

@eonarheim

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

@DavidLi119 Thanks for the contribution!

Responses:

  • The for in this form for (var i = 1; i < 10; i++) { can be safely changed to use let.

  • The tests in VSCode rely on a successful build of the main library, that's why vscode is upset about ../../build/dist/excalibur.

    • run npm install then npm run all to produce a build and run the full test suite. VSCode should find the built files after that.
@eonarheim
Copy link
Member

left a comment

Update for loops to use let and we'll be ready for merge

@DavidLi119

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@eonarheim Changes made. should be good to go no?

@eonarheim
Copy link
Member

left a comment

LGTM 馃殺

@eonarheim eonarheim merged commit 7e96b2c into excaliburjs:master Apr 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 83.288%
Details
@eonarheim

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Thanks for the contribution @DavidLi119!

@jedeen jedeen added this to the 0.23.0 Release milestone Jun 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.