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

modules: Switch from declare to readonly, export #44

Merged
merged 1 commit into from
Dec 20, 2016
Merged

Conversation

mbland
Copy link
Owner

@mbland mbland commented Dec 20, 2016

Encountered in the course of implementing #36. While writing the upcoming @go.log_add_output_file function in thelog module, which includes a call to . "$_GO_USE_MODULES" 'file', a test that made multiple calls to this new function would fail with the message:

 ✗ log/add-output-file: add output files for a mix of levels
   (in test file tests/log/add-output-file.bats, line 83)
     `"@go.log_add_output_file '$TEST_GO_ROOTDIR/info.log' 'INFO'" \' failed
   line 0 not equal to expected value:
     expected: 'INFO   FYI'
     actual:   'No file descriptors <  available; failed at:'

This error message is part of @go.open_file_or_duplicate_fd, and includes a reference to $_GO_MAX_FILE_DESCRIPTORS, which was previously declared as:

  declare -r _GO_MAX_FILE_DESCRIPTORS="${_GO_MAX_FILE_DESCRIPTORS:-256}"

The fact that no value appeared for "$_GO_MAX_FILE_DESCRIPTORS" was suspicous. Moving the . "$_GO_USE_MODULES" 'file' call outside of the function to the top level of the module got the test to pass; I understood this to be because:

  • Top-level declare statements cause variables to be scoped to the
    function sourcing the file.
  • When the function call ends, the declared variables go out of scope.
  • Since "$_GO_USE_MODULES" ensures module files are imported only once,
    these variables aren't redeclared for subsequent calls.

After deciding that it should be permissible for a function to call . "$_GO_USE_MODULES" and have it work across multiple calls, I eventually learned that substituing readonly for declare -r did what I originally intended. The same also holds for export vs declare or declare -x. Making these updates to the file module allowed the test to pass; only minor changes were required to other tests when applying the updates to every module.

For now, this update applies only to module code. I may look into how to apply readonly and export to other code as well, but an initial experiment with replacing values in go-core.bash caused ./go test vars to fail.

While writing the upcoming `@go.log_add_output_file` function in the
`log` module, which includes a call to `. "$_GO_USE_MODULES" 'file'`, a
test that made multiple calls to this new function would fail with the
message:

 ✗ log/add-output-file: add output files for a mix of levels
   (in test file tests/log/add-output-file.bats, line 83)
     `"@go.log_add_output_file '$TEST_GO_ROOTDIR/info.log' 'INFO'" \' failed
   line 0 not equal to expected value:
     expected: 'INFO   FYI'
     actual:   'No file descriptors <  available; failed at:'

This error message is part of `@go.open_file_or_duplicate_fd`, and
includes a reference to `$_GO_MAX_FILE_DESCRIPTORS`, which was
previously declared as:

  declare -r _GO_MAX_FILE_DESCRIPTORS="${_GO_MAX_FILE_DESCRIPTORS:-256}"

The fact that no value appeared for "$_GO_MAX_FILE_DESCRIPTORS" was
suspicous. Moving the `. "$_GO_USE_MODULES" 'file'` call outside of the
function to the top level of the module got the test to pass; I
understood this to be because:

- Top-level `declare` statements cause variables to be scoped to the
  function sourcing the file.
- When the function call ends, the declared variables go out of scope.
- Since "$_GO_USE_MODULES" ensures module files are imported only once,
  these variables aren't redeclared for subsequent calls.

After deciding that it should be permissible for a function to call
`.  "$_GO_USE_MODULES"` and have it work across multiple calls, I
eventually learned that substituing `readonly` for `declare -r` did what
I originally intended. The same also holds for `export` vs `declare` or
`declare -x`. Making these updates to the `file` module allowed the test
to pass; only minor changes were required to other tests when applying
the updates to every module.

For now, this update applies only to module code. I may look into how to
apply `readonly` and `export` to other code as well, but an initial
experiment with replacing values in `go-core.bash` caused `./go test
vars` to fail.
@mbland mbland added the bug label Dec 20, 2016
@mbland mbland added this to the v1.3.0 milestone Dec 20, 2016
@mbland mbland self-assigned this Dec 20, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.435% when pulling fae51c6 on readonly into b61556b on master.

@mbland mbland merged commit af7c110 into master Dec 20, 2016
@mbland mbland deleted the readonly branch December 20, 2016 23:45
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