-
Notifications
You must be signed in to change notification settings - Fork 710
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
lint infinite loop #770
lint infinite loop #770
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! While this technically solves the problem of the infinite loop, it also stops linting test files, which is the intention of lintTest
, and leaves it unused.
Instead I propose modifying lintTest
to stop autofixing by making lintBase
configurable:
const lintBase = (files, options) => {
return src(files)
.pipe($.eslint(options)) // options are passed to ESLint
// ...
}
function lint() {
return lintBase('app/scripts/**/*.js', { fix: true }) // configure autofix
.pipe(dest('app/scripts'));
};
// lintTest stays as-is
What do you think?
Actually, in function lintTest() {
return lintBase('test/spec/**/*.js');
}; |
Perhaps write the lint dest to tmp?
Get Outlook for Android<https://aka.ms/ghei36>
…________________________________
From: Matija Marohnić <notifications@github.com>
Sent: Sunday, February 23, 2020 7:08:56 PM
To: yeoman/generator-webapp <generator-webapp@noreply.github.com>
Cc: Ernest Hope Stephenson <ernie_hs@hotmail.com>; Author <author@noreply.github.com>
Subject: Re: [yeoman/generator-webapp] lint infinite loop (#770)
Actually, in lintTest we would have to lose the dest part to prevent writing the file:
function lintTest() {
return lintBase('test/spec/**/*.js');
};
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#770?email_source=notifications&email_token=ABHZYRWYXVJJVKZDVOCSU3DREK3TRA5CNFSM4KZQP2T2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMWC7FI#issuecomment-590098325>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABHZYRTLMH3NPSYNCFISUULREK3TRANCNFSM4KZQP2TQ>.
|
It would be misleading to write files that we won't use. Is there a reason why you want to write them somewhere? |
Not really I was thinking that they rewrote the files in the editor at times (during infinite loop) however if loop is gone the fine 🙂
Get Outlook for Android<https://aka.ms/ghei36>
…________________________________
From: Matija Marohnić <notifications@github.com>
Sent: Sunday, February 23, 2020 7:34:21 PM
To: yeoman/generator-webapp <generator-webapp@noreply.github.com>
Cc: Ernest Hope Stephenson <ernie_hs@hotmail.com>; Author <author@noreply.github.com>
Subject: Re: [yeoman/generator-webapp] lint infinite loop (#770)
It would be misleading to write files that we won't use. Is there a reason why you want to write them somewhere?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#770?email_source=notifications&email_token=ABHZYRX763PR3WESGFCYQLTREK6S3A5CNFSM4KZQP2T2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMWDVAQ#issuecomment-590101122>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABHZYRTWPD2YYBPG4ZBHGI3REK6S3ANCNFSM4KZQP2TQ>.
|
Great! Are you willing to add these changes to your PR? And leave the |
Hola,
I will have a go implementing these changes
Cheers 🙂
Get Outlook for Android<https://aka.ms/ghei36>
…________________________________
From: Matija Marohnić <notifications@github.com>
Sent: Monday, February 24, 2020 8:27:26 AM
To: yeoman/generator-webapp <generator-webapp@noreply.github.com>
Cc: Ernest Hope Stephenson <ernie_hs@hotmail.com>; Author <author@noreply.github.com>
Subject: Re: [yeoman/generator-webapp] lint infinite loop (#770)
Great, are you willing to add these changes to your PR?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#770?email_source=notifications&email_token=ABHZYRVF2E5EL6EFO4LUT33RENZF5A5CNFSM4KZQP2T2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMW2V2Q#issuecomment-590195434>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABHZYRWK5QYUPII3DS5DO5TRENZF5ANCNFSM4KZQP2TQ>.
|
this seems to work, base is now configurable and stopped writing changes. removed server reload from startTestServer as server reload is in lintBase; no more double reloading.
The browser refresh hppens twice as it occurs in lint base. The server reload part is not needed here it seems.
Ernie
Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows 10
From: Matija Marohnić<mailto:notifications@github.com>
Sent: 27 February 2020 14:50
To: yeoman/generator-webapp<mailto:generator-webapp@noreply.github.com>
Cc: Ernest Hope Stephenson<mailto:ernie_hs@hotmail.com>; Author<mailto:author@noreply.github.com>
Subject: Re: [yeoman/generator-webapp] lint infinite loop (#770)
@silvenon requested changes on this pull request.
In app/templates/gulpfile.js<#770 (comment)>:
@@ -213,8 +212,7 @@ function startTestServer() {
});
watch('app/scripts/**/*.js', scripts);
- watch(['test/spec/**/*.js', 'test/index.html']).on('change', server.reload);
- watch('test/spec/**/*.js', lintTest);
+ watch(['test/spec/**/*.js', 'test/index.html'], lintTest);
Why is this part changed? I suggest returning it to the way it was:
⬇️ Suggested change
- watch(['test/spec/**/*.js', 'test/index.html'], lintTest);
+ watch(['test/spec/**/*.js', 'test/index.html']).on('change', server.reload);
+ watch('test/spec/**/*.js', lintTest);
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#770?email_source=notifications&email_token=ABHZYRQ4WZ3TETYSPKUENGTRE7AL7A5CNFSM4KZQP2T2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXGAZQA#pullrequestreview-365694144>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABHZYRV3MLVR3ZI3IURLJQTRE7AL7ANCNFSM4KZQP2TQ>.
|
looks good, no double reloading etc.. i wonder if it is worth adding package-lock into the .gitignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost… 🤞😉
doh! sorry for that :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
Thank you for spotting this crippling error. Published in v4.0.0-8. 🚀 |
Fixes #769.