Hi Forud, here’s a code review.
Thanks for the MIT license.
Seeing an empty struct type is a code smell to me, but I understand that
this pattern may be required to be a database/sql driver.
The unit tests seem good but I’d add an example or more in-depth overall
test.
In astdata/file.go perhaps consider embedding *File and *Package in type
walker.
I’m not convinced that astdata should be a separate package from
fzerorubigd/goql. Perhaps private types in the primary package would work
fine? My thought is if this isn’t used anywhere else then files like
ast_const.go in goql would be just as readable and clearly not be public
types, and the astdata types could be simplified by avoiding encapsulation.
Do you use astdata anywhere else outside of this project?
Generally I find your code readable with good structure and error handling.
If there’s only one cmd then why not just have a goql/main.go?
In package executor perhaps consider embedding fieldType in type field,
*parse.Query and parse.Stack in type context, parse.ItemType in type dummy,
and parse.Orders in type sortMe. I have the same comment about maybe
eliminating this package.
Using interface{} in executor/filter.go is a code smell but I don’t
understand what’s happening well enough to say if there’s a better way. A
possible suggestion is to put the interface{} behind a named type that
documents what kind of things can be assigned to the interface{}.
I’m not sure I understand the internal packages. Why not have them as part
of package goql too?
This is small, but in package structures func HasFunction you could call
fnLock.RUnlock() right after the read instead of as a defer. More
importantly, are you sure this function isn’t introducing a race condition
where you see a true, the function is unregistered concurrently, then you
try to execute?
I don’t understand the name package structures. Could this one be part of
the primary package too?
You’ve mentioned that documentation is a TODO, but the godoc is definitely
important and to me drives the package architecting.
Matt
On Friday, March 23, 2018 at 5:11:07 AM UTC-5, Forud A wrote:
>
> Hello,
>
> GoQL <https://github.com/fzerorubigd/goql> is a hubby project of mine,
> for extracting global var/func/const/type from go source code using simple
> sql. the project is in early stage and any contribution/advice is welcome!
>
> A Clumsy :) Demo (asscinema) https://asciinema.org/a/170483
> https://github.com/fzerorubigd/goql
>
> Thanks!
>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
