Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generated code output is non-standard #10

Closed
gudvinr opened this issue Apr 4, 2019 · 4 comments
Closed

Generated code output is non-standard #10

gudvinr opened this issue Apr 4, 2019 · 4 comments

Comments

@gudvinr
Copy link

gudvinr commented Apr 4, 2019

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
Copy link
Owner

aaaton commented May 3, 2019

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
Copy link
Owner

aaaton commented May 6, 2019

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.

@aaaton aaaton closed this as completed May 6, 2019
@gudvinr
Copy link
Author

gudvinr commented May 7, 2019

@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
Copy link
Owner

aaaton commented May 7, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants