goawk
goawk copied to clipboard
Track filenames parsed
This is the other PR in context of #144.
Here I tried to address the item 1. of your list @benhoyt.
Eventually I've found semi-elegant solution of how to simplify the position-to-file tracking.
So the idea is (using ast.Walk()
functionality that we now have) to keep mapping of Node -> filename for each AST node during parsing individual files. Thus we can resolve the error position back to the file by relying on the node where the error happens.
This approach also allowed to get rid of the errorFileLine
function, as expected.
The solution is almost final and test suite is passing. I'm working on final refactorings but would like you to take a look @benhoyt.
Ok, I think I'm done.
Hi @xonixx, sorry for not replying sooner. This approach is along the lines of what I was thinking originally. However, on reflection it's quite invasive into the existing parsing code and adds exported API surface that I'm not entirely sure about. Plus, it uses a big map of nodes, which is potentially a decent amount of memory usage for larger programs -- ideally we'd only have one item tracked per file.
I was mulling this over and playing with an approach for this the other day, and here's what I came up with: https://github.com/benhoyt/goawk/pull/151/files
It's an internal package for a start, so we don't export the API and we don't have to get the API exactly right the first time. The new type is FileReader
, on which you call AddFile
for every file you add, and it reads the file and tracks the path name and number of lines in each file (one item tracked per file, so tiny memory footprint). Then you call FileLine
with the overall line number and it returns the file path and file-relative line number.
So it gets rid of errorFileLine
in the same way, and hopefully can also be used for coverage file paths/line numbers.
I'm not sure I love the name FileReader
, but I'm open to suggestions there.
Thoughts?
Note that my PR isn't intended for merging as is. I haven't gone over it in details, and it needs comments and tests. I'm happy for you to tidy it up and get it ready for merging if you'd like. We just need to make sure the approach will work for your upcoming coverage handling as well.
Thanks, this looks simpler indeed. I’ll work on it.