-
Notifications
You must be signed in to change notification settings - Fork 78
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
release/0.16.0 #1119
release/0.16.0 #1119
Conversation
28e8990
to
e758c7b
Compare
e758c7b
to
37974b9
Compare
b94bc6e
to
a204a9f
Compare
writeFileSync(jsonPath, JSON.stringify(object, null, 2) + '\n') | ||
} | ||
|
||
const fromEntries = (xs: [string | number | symbol, any][]) => |
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.
I think it's more clear if the argument of the function is a dictionary.
in any case, since this function is only used once and it's not clear what it does without a context, I would embed it where you use it. Btw, why not embedding the code directly within the readJson function?
const spawnProcess = async ( | ||
command: string, | ||
args: string[] = [], | ||
log?: { onSuccess?: any } |
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.
change any to () => void
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.
most times it's better to abort after a process fails to avoid mixing up too many error messages (you can leave the console.error and rethrow)
I suggest getting rid of the onSuccess and just directly calling what you calls from onSuccess after the spawnProcess (since you can always do await).
Another option would be that you pass a successMessage to spawnProcess, so that you don't need to repeat "console.log(greeen" so often
@dpinol I will refactor this script into another PR ASAP once this has been merged, as atm is not necessary for releasing the new versions. |
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.
LGTM
d937f77
to
fda70ab
Compare
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.
Nice script 👏!!
Description
ts-node publish.ts