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

Enhancements #531

Merged
merged 6 commits into from Jan 28, 2020
Merged

Enhancements #531

merged 6 commits into from Jan 28, 2020

Conversation

jamesgeorge007
Copy link
Member

@jamesgeorge007 jamesgeorge007 commented Jan 27, 2020

Follow up of #518

  • Typo fixes.
  • Couple of other tweaks.

@ghost
Copy link

ghost commented Jan 27, 2020

DeepCode's analysis on #f93fc4 found:

  • 0 critical issues. ⚠️ 0 warnings and 0 minor issues. ✔️ 0 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

@jamesgeorge007 jamesgeorge007 added core Changes regarding core concepts refactor Code refactoring labels Jan 27, 2020
@jamesgeorge007 jamesgeorge007 added this to the v2.0 milestone Jan 27, 2020
liyasthomas
liyasthomas previously approved these changes Jan 27, 2020
@liyasthomas
Copy link
Member

liyasthomas commented Jan 27, 2020

I suspect @nickpalenchar planned on a nested tests kinda thing for this. At least those naming looks like that. Anyway, only merge at Nick's review approval.

@TravisBuddy
Copy link

Hey @jamesgeorge007,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: d0e3e9d0-4109-11ea-a60c-ed450572c3ce

Copy link
Contributor

@nickpalenchar nickpalenchar left a comment

Choose a reason for hiding this comment

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

Thanks for these changes @jamesgeorge007, especially with proofreading my frantic typing mishaps! This change sacrifices some readability of the testing API. toBeLevel(200) sounds like a status should be that exactly when reading it like English. Furthermore, it increases margin for accidental false positives. E.g if a user tried with toBeLevel(404), the test would pass if the server 500'd in most situations. the toBeLevelxxx methods are higher-level convince and are unlikely to scale past 4 different ranges anytime soon in the land of http. I'm fine with the redundancy at that scale, as well as finding ways to DRY it up, but it should not sacrifice obvious readability or simple usage.

The corrections of my js misusage (let -> const, typos) should def get merged regardless.

liyasthomas
liyasthomas previously approved these changes Jan 27, 2020
@TravisBuddy
Copy link

Hey @jamesgeorge007,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: aa57e3e0-412a-11ea-a60c-ed450572c3ce

@TravisBuddy
Copy link

Hey @jamesgeorge007,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: d5f226f0-412a-11ea-a60c-ed450572c3ce

@TravisBuddy
Copy link

Hey @jamesgeorge007,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: fd5ddb80-412a-11ea-a60c-ed450572c3ce

@TravisBuddy
Copy link

Hey @jamesgeorge007,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 51c4f780-4130-11ea-a60c-ed450572c3ce

@liyasthomas liyasthomas merged commit d707ba3 into master Jan 28, 2020
@jamesgeorge007 jamesgeorge007 deleted the enhancements branch January 28, 2020 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes regarding core concepts refactor Code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants