Hi Matt, First, Thank you!
On Saturday, March 24, 2018 at 10:37:57 PM UTC+4:30, matthe...@gmail.com wrote: > > 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. > > It must implement an interface, so `a type` is needed. I have plans to add some option, for example enable/disable cache, load external function result (for example in var x = flags.String(....) , to detect type of x, must load the package flags) and ... so the struct is fine here :) The unit tests seem good but I’d add an example or more in-depth overall > test. > > Yes, you are right. need more in depth tests, I need some time to do it, or some contribution :)) In astdata/file.go perhaps consider embedding *File and *Package in type > walker. > > I have nothing against it :) OK 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? > > This package is used in many of my code generators, its actually based on my other package: https://github.com/fzerorubigd/humanize I want to retire that, and maintain only one package :) also my current company code is depend on that structure. 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? > > Not yet, but you are right. 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. > Not agains this one :) > 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{}. > > the executor was a "try" and my first try is this. I have plan to refactor it, but after the main goal is achieved. I’m not sure I understand the internal packages. Why not have them as part > of package goql too? > Yes, runtime is redundant and should be inside the goql package. parse is some generic sql parser (with limited functionality) and it is usable without GoQL, so I prefer to keep it in separate package, but outside of internal. > > > 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? > > :/ it is deffered : https://github.com/fzerorubigd/goql/blob/c2539db26fd2ec077f8f68ce413b30dceca72195/structures/functions.go#L33 > I don’t understand the name package structures. Could this one be part of > the primary package too? > > It is a generic package to create a table like structure, extracted from one of my (closed source) codes. the name is bad, choosing a name for a package is hard :)))) so anyone with a better name is a great help! > > You’ve mentioned that documentation is a TODO, but the godoc is definitely > important and to me drives the package architecting. > Yes, and you are right again :) > > > Matt > Thank you once more! > > 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 golang-nuts+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
