search

aaaton / golem

A lemmatizer implemented in Go
MIT License
85 stars 19 forks source link

Generated code output is non-standard #10

Closed gudvinr closed 5 years ago

gudvinr commented 5 years ago

Consider using standard tags in generated dictionaries which has standard format according to this issue.

Also, since your dictionaries are placed in same package it'll be move convenient to generate them to different files rather than single one. That'll help to track changes if necessary since these dictionaries are relatively big.

aaaton commented 5 years ago

Thanks for your suggestions. I'll look into both.

I think the first suggestion might just be a matter of updating the data generator used and re-generating. But the are some other details I would like the fix with that as well.

Someone suggested adding the different languages to different packages and requiring the import of those packages for the language to work, which I think makes sense, performance and memory usage wise. And the git tracking like you suggested.

There should also be some way of reducing the storage size by at least a factor 2 for the language-files. Which should also affect tracking capabilities.

aaaton commented 5 years ago

Should be solved by v2.0 I'm using go-bindata to generate the dict files, and the generated code-comment seems to be multiline which I think github may have issues with. But that is a problem with go-bindata, rather than golem.

Split into both separate files and multiple packages.

gudvinr commented 5 years ago

@aaaton yeah, that's cool. While original issue considered solved I'd like to make some notes. I did a quick peek at your code and if it's appropriate for you there's things that can be improved:

  1. I don't see need for having exported struct for LanguagePack and since you have generic interface for that anyway it may be convenient to return this one instead of struct pointer. But it's questionable though. And interface definition in this case should be moved from golem package to avoid circular dependencies (root of dict/ is sufficient I guess).
  2. You have some common code inside your generated language files which probably can be exported to another package and re-used. It'll help to avoid errors when you need to regenerate these files.
  3. As a general recommendation it is good to name methods that create objects just "New" in these cases where package can return only single type of object.
  4. Comments needed to guard generated files still seem not to be respected by code editors (single-line comment with dot at the end should be used I suppose).
aaaton commented 5 years ago

Thanks for your feedback @gudvinr.

I think you have a few sensible points there. But I have questions!

  1. Whats's the argument for exporting the interface instead of a struct implementing the interface?

  2. The duplicated code is sad indeed. I'll look into this.

  3. This was my original design, but felt confusing to write en.New() to produce a LanguagePack. That pattern makes way more sense when the package name accurately describes the single purpose of the constructor. Like golem.New(). But perhaps consistency outweighs descriptiveness.

  4. Yeah, the pattern Rob Pike posted is the one I've added to the generated files of the language/pack.go structures. That seems to work for VS Code and GitHub for me at least. Does it not work for you? The language/data.go files have the "generation-comment" produced by go-bindata which is not really something I want to alter.