-
Notifications
You must be signed in to change notification settings - Fork 994
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
chore(jobs): Mostly nitpicky changes to the Jobs PR #11328
Changes from 2 commits
a98178c
50eed77
74b59df
8e7911f
8faad72
d79f8cb
a7beacb
29474a6
bf06c06
fec949a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,8 @@ import { prepareForRollback } from '../../../lib/rollback' | |
import { yargsDefaults } from '../helpers' | ||
import { validateName, templateForComponentFile } from '../helpers' | ||
|
||
// Makes sure the name ends up looking like: `WelcomeNotice` even if the user | ||
// called it `welcome-notice` or `welcomeNoticeJob` or anything else | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just thought "anything else" was maybe a little too over reaching 😆 |
||
// Try to make the name end up looking like: `WelcomeNotice` even if the user | ||
// called it `welcome-notice` or `welcomeNoticeJob` or something like that | ||
const normalizeName = (name) => { | ||
return changeCase.pascalCase(name).replace(/Job$/, '') | ||
} | ||
|
@@ -107,7 +107,8 @@ export const description = 'Generate a Background Job' | |
// This could be built using createYargsForComponentGeneration; | ||
// however, functions shouldn't have a `stories` option. createYargs... | ||
// should be reversed to provide `yargsDefaults` as the default configuration | ||
// and accept a configuration such as its CURRENT default to append onto a command. | ||
// and accept a configuration such as its CURRENT default to append onto a | ||
// command. | ||
export const builder = (yargs) => { | ||
yargs | ||
.positional('name', { | ||
|
@@ -137,7 +138,8 @@ export const builder = (yargs) => { | |
)}`, | ||
) | ||
|
||
// Add default options, includes '--typescript', '--javascript', '--force', ... | ||
// Add default options. This includes '--typescript', '--javascript', | ||
// '--force', ... | ||
Object.entries(yargsDefaults).forEach(([option, config]) => { | ||
yargs.option(option, config) | ||
}) | ||
|
@@ -156,7 +158,7 @@ export const handler = async ({ name, force, ...rest }) => { | |
|
||
let queueName = 'default' | ||
|
||
// Attempt to read the first queue in the users job config file | ||
// Attempt to read the first queue in the user's job config file | ||
try { | ||
const jobsManagerFile = getPaths().api.distJobsConfig | ||
const jobManager = await import(pathToFileURL(jobsManagerFile).href) | ||
|
@@ -170,9 +172,8 @@ export const handler = async ({ name, force, ...rest }) => { | |
{ | ||
title: 'Generating job files...', | ||
task: async () => { | ||
return writeFilesTask(await files({ name, queueName, ...rest }), { | ||
overwriteExisting: force, | ||
}) | ||
const jobFiles = await files({ name, queueName, ...rest }) | ||
return writeFilesTask(jobFiles, { overwriteExisting: force }) | ||
}, | ||
}, | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,7 +95,7 @@ const tasks = async ({ force }) => { | |
fs.mkdirSync(getPaths().api.jobs) | ||
} catch (e) { | ||
// ignore directory already existing | ||
if (!e.message.match('file already exists')) { | ||
if (!/file already exists/.test(e.message)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I hate this syntax, it feels backwards to me. In real life I'd say "Is this car red?" Not "Is the color red what this car is?" GROSS If you start with a regex object (because it was passed in as an argument, or you constructed the regex from other variables) you can use But if it's more performant or whatever, we can make it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't argue with that... I also think it reads backwards 🙁 But it is the recommended way! |
||
throw new Error(e) | ||
} | ||
} | ||
|
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.
Following our code standards of only using underscore (
_foo
) for unused variables