-
Notifications
You must be signed in to change notification settings - Fork 31
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
Refactore index.js to be able to be required programmatically #7
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR! It's a huge refactor so let me take a close look at it :) Btw what do you mean by |
src/api.js
Outdated
const promises = events.map(async event => { | ||
return await insert(event); | ||
}); | ||
return await pSettle(promises); |
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.
return await
is not performant since it's redundant, just doing return pSettle(promises);
is better.
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.
You're right. I'll update the MR.
module.exports.bulk = async events => { | ||
const calendar = await client(); | ||
const insert = promisify(calendar.events.insert); | ||
const promises = events.map(async event => { |
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.
const promises = events.map(async event => insert(event));
Is better I think.
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 we should write also const promises = events.map(async event => insert(event));
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.
what do you mean?
I meant:
const promises = events.map(event => insert(event));
async
is not necessary.
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.
Got it.
package.json
Outdated
"dependencies": { | ||
"babel-eslint": "^9.0.0", |
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.
What did you need it for? btw I think it should go in devDependencies
.
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.
It's due, i think, to my IDE.
I'll remove the installation.
I edited also the description and the MR title to be explicit about the programmatically way. |
6c561e7
to
1e6fa9a
Compare
The next step will be to upgrade dependencies and move |
Regarding all refactoring and new features, we could also open the PR to a dedicated branch. |
Since this is quite big I think that's a good idea! We could separate it in several PRs to a dedicated branch, and at the end merge that branch with master in a final PR. |
I created a branch called 2.0.0. This order could be nice, so we can isolate possible bugs:
|
0a8a9b0
to
055ec96
Compare
What?
This MR contains largely a smooth refactoring to use gcal-cli also in a programmatically way.
How?
bin.js
api.js
index.js
Do directives
doc for the first authorization installationPromise.all