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

fix: fall back to default logs path in getPath('logs') #19653

Merged
merged 5 commits into from
Aug 19, 2019

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Aug 6, 2019

Description of Change

Follow-up to #19545 (comment).

If users try to call app.getPath('logs') before they've called app.setAppLogsPath() then it would previously error. Apps that relied on getPath('log') to work would get an exception, which is not great API.

Now, calling app.getPath('logs') without having previously called app.setAppLogsPath() will result in a directory being created and set to /Library/Logs/YourAppName on macOS, and inside the userData directory on Linux and Windows.

This also adds a converter for base::Optional<T> such that we don't introduce the possibility for mate::Arguments* pollution a la https://xkcd.com/327.

cc @MarshallOfSound

Checklist

Release Notes

Notes: Fixed an issue where app.getPath('logs') would throw an error if the logs path was not previously set.

@electron-cation electron-cation bot added new-pr 🌱 PR opened in the last 24 hours and removed new-pr 🌱 PR opened in the last 24 hours labels Aug 6, 2019
@codebytere codebytere requested review from ckerr and jkleinsc August 7, 2019 17:01
@codebytere codebytere force-pushed the setpath-logs-default branch from 0e2488d to aae5648 Compare August 7, 2019 22:12
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

The idea looks fine. A couple of nits in comments.

shell/browser/api/atom_api_app.cc Outdated Show resolved Hide resolved
shell/browser/api/atom_api_app.cc Outdated Show resolved Hide resolved
docs/api/app.md Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the setpath-logs-default branch 7 times, most recently from b6996b1 to 6a17122 Compare August 13, 2019 21:10
docs/api/app.md Outdated Show resolved Hide resolved
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Oh actually one final thing, can we put like a test for this somewhere 😆

@codebytere codebytere force-pushed the setpath-logs-default branch from f3eada6 to 5b26197 Compare August 19, 2019 19:53
@codebytere
Copy link
Member Author

@MarshallOfSound should be all set now!

@codebytere codebytere merged commit 1dc02e6 into master Aug 19, 2019
@release-clerk
Copy link

release-clerk bot commented Aug 19, 2019

Release Notes Persisted

Fixed an issue where app.getPath('logs') would throw an error if the logs path was not previously set.

@codebytere codebytere deleted the setpath-logs-default branch August 19, 2019 22:16
@trop
Copy link
Contributor

trop bot commented Aug 19, 2019

I was unable to backport this PR to "6-0-x" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Aug 19, 2019

I have automatically backported this PR to "7-0-x", please check out #19836

@trop
Copy link
Contributor

trop bot commented Aug 26, 2019

A maintainer has manually backported this PR to "6-0-x", please check out #19955

return true;
}
T converted;
if (Converter<T>::FromV8(isolate, val, &converted)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@codebytere I just ran into this code when doing refactoring. I believe this should be if not otherwise the out would never be filled?

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.

4 participants