search

mna / pigeon

Command pigeon generates parsers in Go from a PEG grammar.
BSD 3-Clause "New" or "Revised" License
822 stars 66 forks source link

remove ioutil (fixes #106) #108

Closed flowchartsman closed 1 year ago

flowchartsman commented 1 year ago

Fixes #106

breml commented 1 year ago

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.

flowchartsman commented 1 year ago

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 commented 1 year ago

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.