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

remove ioutil (fixes #106) #108

Merged
merged 2 commits into from Jun 7, 2023
Merged

remove ioutil (fixes #106) #108

merged 2 commits into from Jun 7, 2023

Conversation

flowchartsman
Copy link
Contributor

Fixes #106

@breml
Copy link
Collaborator

breml commented Mar 13, 2023

Thanks @flowchartsman for this PR as well as for #109. I have been very hesitant in adopting new version of Go with this package. Replacing io/ioutil as well as replacing interface{} with any will basically break pigeon on older versions of the Go compiler. Therefore I ask myself the following things:

  1. Is it ok for pigeon to just adopt these newest changes in Go and make it break for users with older version of the Go compiler?
  2. Should pigeon stay backwards compatible by using build flags?
  3. Should pigeon offer a flag to generate up-to-date code (without io/ioutil and with any instead of interface{})?
  4. Should pigeon detect the compiler version used and based on this generate different code?

I have not yet come to a conclusion and therefore I am not yet ready to review this PR nor merging it. So far, "breaking" changes have in this area have only been made for the introduction of Go modules (see the README.md). That being said, I think this PR would need to update the go.mod and the README.md files as well.

Additionally, the automated CI is no longer working (it still relies on travisci) and this should be moved to Github actions before we update to more recent version of Go.

@breml breml mentioned this pull request Mar 13, 2023
@flowchartsman
Copy link
Contributor Author

I have not yet come to a conclusion and therefore I am not yet ready to review this PR nor merging it. So far, "breaking" changes have in this area have only been made for the introduction of Go modules (see the README.md). That being said, I think this PR would need to update the go.mod and the README.md files as well.

I'll probably throw that on, but, at least with this PR, my attitude is: the issue is still open, so now it has a PR. You can close it (or not) as you like, but now there's a whole PR if you want it. I just happen to be using Pigeon in a project right now, and the lint errors were annoying me. :) Given the Go Compatibility guaranteed, it really doesn't matter, though I will say the code is a heck of a lot more pleasant to look at with #109 in place.

@flowchartsman
Copy link
Contributor Author

Additionally, the automated CI is no longer working (it still relies on travisci) and this should be moved to Github actions before we update to more recent version of Go.

Sure. I mean, I ran all the tests in a couple versions of Go, but it's easy enough to move to Github actions.

@flowchartsman flowchartsman mentioned this pull request Mar 14, 2023
Copy link
Collaborator

@breml breml left a comment

Choose a reason for hiding this comment

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

LGTM

@breml breml merged commit ef65bd5 into mna:master Jun 7, 2023
9 checks passed
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.

Remove dependency of io/ioutils
2 participants