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.

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

Fixed pad disable detection flow #1312

Merged
merged 3 commits into from
Oct 27, 2017
Merged

Fixed pad disable detection flow #1312

merged 3 commits into from
Oct 27, 2017

Conversation

robertohuertasm
Copy link
Member

@robertohuertasm robertohuertasm commented Oct 26, 2017 •
edited

Fixes #1311

Changes proposed:

  • Add
  • Prepare
  • Delete
  • Fix

Things I've done:

  • My pull request fixes an issue, I referenced the issue.

@robertohuertasm robertohuertasm added this to the Next milestone Oct 26, 2017
@robertohuertasm robertohuertasm requested a review from a team October 26, 2017 22:58
@codecov
Copy link

codecov bot commented Oct 27, 2017 •
edited

Codecov Report

Merging #1312 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1312   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          47      47           
  Lines        2558    2558           
  Branches      129     129           
======================================
  Hits         2558    2558
Impacted Files Coverage Δ
src/init/projectAutoDetection.ts 100% <100%> (ø) ⬆️
test/init/projectAutoDetection.test.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b9a154...658be89. Read the comment docs.

@JimiC
Copy link
Member

JimiC commented Oct 27, 2017 •
edited

This happened because in the latest iteration I removed results.length check in detectAngular and then I forgot to change the return value of detectProject. This is the price we pay for not completely covering the project with BDD test cases. I'm to blame for both.

JimiC
JimiC reviewed Oct 27, 2017
View reviewed changes
test/init/projectAutoDetection.test.ts Outdated
@@ -67,7 +67,9 @@ describe('AutoDetectProject: tests', function () {
function () {
userConfig.projectDetection.disableDetect = true;
return pad.detectProject(null, userConfig)
.then(res => expect(res).to.be.an('array'));
.then(res => {
Copy link
Member

Choose a reason for hiding this comment

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

@robertohuertasm Is this your prefered coding style?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I was using this to debug and later I forgot to go back to a one line style. It was late... 😉

@robertohuertasm robertohuertasm changed the title fixed pad disable detection flow Fixed pad disable detection flow Oct 27, 2017
@robertohuertasm robertohuertasm merged commit f71ea4a into master Oct 27, 2017
@robertohuertasm robertohuertasm deleted the 1311-fix-pad-disable-detection branch October 27, 2017 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JimiC JimiC JimiC approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
7.17.0
Development

Successfully merging this pull request may close these issues.

Project detection popup every window reload
2 participants
@robertohuertasm @JimiC