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

interp: Runner exports some unneccessary fields #428

Closed
kaey opened this issue Oct 4, 2019 · 1 comment · Fixed by #433
Closed

interp: Runner exports some unneccessary fields #428

kaey opened this issue Oct 4, 2019 · 1 comment · Fixed by #433

Comments

@kaey
Copy link
Contributor

kaey commented Oct 4, 2019

Docs say that fields are read-only, which means following fields should be unexported:

Exec ExecModule
Open OpenModule

// And arguably these too.
Stdin  io.Reader
Stdout io.Writer
Stderr io.Writer

There is interesting case with exec 2>/some/file though, which I don't know how should be handled and exposed yet.

I would also consider actually unexporting all fields and replacing them with runner.State() *interp.RunnerState method. This makes api intent slightly clearer at the cost of adding new struct type, but I'm not sure whether such change is useful or not.

I think this is my last itch with current api, there are other problems, but they don't touch public api and can be fixed later.

@mvdan
Copy link
Owner

mvdan commented Oct 4, 2019

I agree on your first point there. I think these were exposed from the beginning as I didn't add the constructor options until a bit later.

I still think a separate state wouldn't be a good idea overall. I get what you're trying to get at, but what would runner.State() do?

  • If it copies values, it's going to be slow - particularly if one wants just one field
  • If it returns the values directly, then it's not really any extra protection

I think the extra godoc to clarify what the fields are for is good enough for this purpose.

@kaey kaey changed the title interp: Runner exports some unneccessary field interp: Runner exports some unneccessary fields Oct 8, 2019
@mvdan mvdan closed this as completed in #433 Oct 9, 2019
mvdan pushed a commit that referenced this issue Oct 9, 2019
These fields are supposed to be modified via calling options
and there is little reason to read from them.

Fixes #428
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 a pull request may close this issue.

2 participants