pluggable code generators - Githubissues

tdenniston / bish

Bish is a language that compiles to Bash. It's designed to give shell scripting a more comfortable and modern feel.

MIT License

1.48k stars 36 forks source link

pluggable code generators #37

Closed digitalsurgeon closed 9 years ago

digitalsurgeon commented 9 years ago

bash
zsh ( non working only skeleton )

I am thinking of writing a zsh implementation :)

tdenniston commented 9 years ago

Good idea. I'll review this and get back to you hopefully by tomorrow.

tdenniston commented 9 years ago

Ok, this is a good idea, but there are a couple of things that should change before I merge it in. The big thing is to submit a zsh implementation in a separate PR, instead of adding the skeleton implementation now. Otherwise, there are just some small style issues that should be fixed.

Implementation of zsh should happen in a separate pull request.
Since zsh is unimplemented, remove the zsh-related files and arguments for now.
Remove the createInstance method and just use new CodeGen_Bash(...) unless you can come up with a rationale otherwise.
Rename your Cons typedef to something more descriptive.
Fix a few whitespace inconsistencies (e.g. the case clauses shouldn't have newlines in between, as in the rest of the codebase).

digitalsurgeon commented 9 years ago

Implementation of zsh should happen in a separate pull request.
- ok
Since zsh is unimplemented, remove the zsh-related files and arguments for now.
- ok
Remove the createInstance method and just use new CodeGen_Bash(...) unless you can come up with a rationale otherwise.
- The reason I am not using "new" is to not allocate the generator objects needlessly. createInstance allows my to specify a constructor function to create only the object specified with -u command line.
Rename your Cons typedef to something more descriptive.
- ok
Fix a few whitespace inconsistencies (e.g. the case clauses shouldn't have newlines in between, as in the rest of the codebase).
- ok

tdenniston commented 9 years ago

Thanks for the changes. A couple more small issues before it's ready to merge:

I think you meant CodeGeneratorConstructor not ConGeneratorConstructor
createInstance: I understand now why you did it this way (and it's a neat idea). But, since it wasn't clear to me how it worked, it would be good to have a brief comment in the code explaining it. Also, can you rename it to create_instance to fit the codebase style?
const std::string& sh should be const std::string &sh, since that's how the rest of the codebase is.
Remove your debug message about which code generator is being used.
The error message about using an unknown code generator should got to std::cerr and not std::cout.

Sorry for the nitpicking, but I like to have clear code and consistent style throughout. If we make the effort to keep things clear and consistent now, when the codebase is still small, we'll be much better off in the future as it grows in size. I do appreciate your contributions though.

tdenniston commented 9 years ago

@digitalsurgeon Sorry, I didn't realize you had updated this with the review comments! I didn't get a notification about the commit you added. I'll fix up the conflicts and get this merged.

tdenniston commented 9 years ago

Ok, all set. I fixed up a few more style inconsistencies: hope you don't mind.