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

refactor: make app logs dir creation opt-in #17841

Merged
merged 2 commits into from Apr 19, 2019

Conversation

@codebytere
Copy link
Member

commented Apr 17, 2019

Description of Change

BREAKING CHANGE

Resolves #17732.
Resolves #11858.
Resolves #15877.
Resolves #14470.

Previously, we were creating the app logs folder at a predefined location during initial electron startup, which meant that it had to be manually removed and prevented clean app portability. This refactors that implementation such that it's now an opt-in feature and developers must call app.setAppLogsPath() in order to set this directory.

Alternatively, this could be done such that app log dir creation is still enabled by default, but can be disabled at the developers discretion although this would be notably more challenging. I'd love some feedback on which approach seems optimal!

cc @zcbenz @deepak1556

Checklist

Release Notes

Notes: Made app log directory creation opt-in with a new function app.setAppLogsPath.

@codebytere codebytere requested a review from deepak1556 Apr 17, 2019

@codebytere codebytere force-pushed the no-auto-logs branch from ed48e4a to 5236c36 Apr 17, 2019

@codebytere codebytere requested a review from zcbenz Apr 18, 2019

@zcbenz

zcbenz approved these changes Apr 18, 2019

@electron-cation electron-cation bot removed the new-pr 🌱 label Apr 18, 2019

@deepak1556
Copy link
Member

left a comment

LGTM, with some clarification

Show resolved Hide resolved atom/browser/api/atom_api_app.cc Outdated

@codebytere codebytere force-pushed the no-auto-logs branch from 5236c36 to 3d69de4 Apr 18, 2019

@codebytere codebytere requested a review from deepak1556 Apr 18, 2019

@codebytere codebytere force-pushed the no-auto-logs branch from 3d69de4 to e53a365 Apr 18, 2019

Show resolved Hide resolved atom/browser/api/atom_api_app.cc Outdated

@codebytere codebytere force-pushed the no-auto-logs branch from e53a365 to 362ab0f Apr 19, 2019

@codebytere codebytere force-pushed the no-auto-logs branch from 362ab0f to b6ca6f5 Apr 19, 2019

@codebytere codebytere merged commit 0749dc4 into master Apr 19, 2019

13 of 15 checks passed

appveyor: win-ia32-testing Waiting for AppVeyor build to complete
Details
Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190419.3 succeeded
Details
electron-arm64-testing Build #20190419.3 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Apr 19, 2019

Release Notes Persisted

Made app log directory creation opt-in with a new function app.setAppLogsPath.

@codebytere codebytere deleted the no-auto-logs branch Apr 19, 2019

@vladimiry

This comment has been minimized.

Copy link

commented Apr 26, 2019

Is it going to land in v5 any time soon? v5.0.0 didn't get this change.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@vladimiry This PR is tagged as semver/major and therefore won't go out until a major release happens (the next one being 6.0)

Kiku-Reise added a commit to Kiku-Reise/electron that referenced this pull request May 16, 2019

refactor: make app logs dir creation opt-in (electron#17841)
Previously, we were creating the app logs folder at a predefined location during initial electron startup, which meant that it had to be manually removed and prevented clean app portability. This refactors that implementation such that it's now an opt-in feature and developers must call app.setAppLogsPath(path) with an optional custom path in order to set this directory.

@DragDay7 DragDay7 referenced this pull request Jul 31, 2019

Closed

electron 6: app.getPath('logs') throws error on Windows #19543

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.