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.

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

Replace download-chromium with puppeteer-core #178

Merged
merged 3 commits into from
Jan 3, 2021

Conversation

baruchiro
Copy link
Collaborator

@baruchiro baruchiro commented Jan 1, 2021

Still have problems with the Windows installed version. I'm getting errors from download-chromium when it uses ncp. We don't need that package anymore.

Reference: juliangruber/download-chromium#23

Still related to #170

@baruchiro baruchiro requested a review from brafdlog January 1, 2021 14:18
(cherry picked from commit 423f071)
(cherry picked from commit 359ac7f)

# Conflicts:
#	package.json
#	yarn.lock
@baruchiro baruchiro force-pushed the baruchiro/issue170-1 branch from 359ac7f to edf5db0 Compare January 2, 2021 18:24
Comment on lines +17 to +24
:rules="[rules.required]"
@change="changed = true"
/>
<v-text-field
v-model="exporter.options.budgetId"
label="Budget id"
outlined
:rules="rules.required"
:rules="[rules.required]"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix bug

Comment on lines 28 to 34
export enum AccountStatus {
IDLE, PENDING, IN_PROGRESS, DONE, ERROR
IDLE = 'IDLE',
PENDING = 'PENDING',
IN_PROGRESS = 'IN_PROGRESS',
DONE = 'DONE',
ERROR = 'ERROR'
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

More readable in the console, so why not

@baruchiro
Copy link
Collaborator Author

@brafdlog It is not including another PR now, you can review.

@baruchiro
Copy link
Collaborator Author

@brafdlog Note I enabled Auto-Merge, I don't know exactly what is the procedure, but it is mean that if you're commenting a suggestion and then approving my PR, it will merge automatically.

This is something to consider. I think in my company we are enabling Auto Merge only on already approved PRs, just for auto merging after the validation checks.

@baruchiro baruchiro merged commit f92b01e into master Jan 3, 2021
@baruchiro baruchiro deleted the baruchiro/issue170-1 branch January 3, 2021 15:13
@brafdlog
Copy link
Owner

brafdlog commented Jan 3, 2021

@baruchiro I am used to approving prs with comments if in general I accept the pr except for minor changes. The benefit of this is that it doesn't require another review which can be a redundant blocker.

So we can either stay with auto merge and only approve when all is well or disable auto merge.

I think we should disable auto merge because waiting for another review on minor changes can take a few days if we are busy and I don't think that is worth it. WDYT?

@baruchiro
Copy link
Collaborator Author

Yeah, sure, I totally agree with you. I just wanted to check this feature :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants