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: don't expose tempdir in common.state #903

Merged
merged 1 commit into from Nov 9, 2018

Conversation

nfischer
Copy link
Member

@nfischer nfischer commented Nov 8, 2018

Previously, the cached tempdir value was stored in common.state.
Unlike the other common.state values, this isn't immediately useful to
other commands (they can just call the tempdir API). So, this moves the
cached value into tempdir.js.

This also adds a unit test for the caching behavior, and exposes
test-only helpers to verify this behavior.

Finally, this adds a note to common.state that values should generally
be considered read-only, since this can be important for customized
behavior. Although, I recognize our code base has one exception to this
rule (echo()), we should strive to maintain this.

Fixes #902
Test: Added a unit test.

Previously, the cached `tempdir` value was stored in `common.state`.
Unlike the other `common.state` values, this isn't immediately useful to
other commands (they can just call the tempdir API). So, this moves the
cached value into `tempdir.js`.

This also adds a unit test for the caching behavior, and exposes
test-only helpers to verify this behavior.

Finally, this adds a note to `common.state` that values should generally
be considered read-only, since this can be important for customized
behavior. Although, I recognize our code base has one exception to this
rule (`echo()`), we should strive to maintain this.

Fixes #902
Test: Added a unit test.
@nfischer
Copy link
Member Author

@nfischer nfischer commented Nov 8, 2018

@wyardley interested in doing a code review?

@nfischer
Copy link
Member Author

@nfischer nfischer commented Nov 9, 2018

Thanks!

@nfischer nfischer merged commit 6b3c7b1 into master Nov 9, 2018
3 of 4 checks passed
@nfischer nfischer deleted the refactor-dont-expose-temp branch Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants