search

psyomn / agen

Dumb code generation tool for Ada
Apache License 2.0
1 stars 0 forks source link

Restructuring/Review #6

Closed Entomy closed 5 years ago

Entomy commented 5 years ago

I've restructured the project a bit, since typically speaking parent packages should not depend on their children. The parsing model has been changed to something much more robust, specifically a stack-based try parser that calls the appropriate library function. Code has been cleaned up and conforms better to best practices, such as string comparisons through uppercase conversions.

There's considerably more error messages and more helpful error messages, as well as an (internal) elimination of exception handlers as these are extremely expensive.

I've done my best to keep to the established style, although I've done this in multiple sittings so I probably broke that several times. Reformat as you see fit.

Closes #1 Closes #3

Entomy commented 5 years ago

This also fixes Issues #1 and #3

Entomy commented 5 years ago

I'm not surprised by whitespacing issues. I use tabs normally with Ada, and was trying to remember to use spaces instead. I can try rebasing, but as it's an unfamiliar approach I might mess it up.

I don't mind a name change. Might be a good idea since "gnatgen" seems to be a core GNAT component. Also sort of makes it seem like it's GNAT specific which it isn't and shouldn't be. Generating GNAT specific aspects, attributes, or pragmas seems like generating overly specific code.

Entomy commented 5 years ago

I'll be adding some more linting flags on the next iteration.

On a somewhat related note, a file that I can add that is a standard, but not universally supported .editorconfig can enforce some style guidelines in editors that support it. Linting (gnatpp probably?) is certainly a great idea. .editorconfig can help enforce these ahead of time; at least force me to be using spaces for this project. As you said you didn't want specific tooling, I'll add it to the .gitignore if you don't want it included.

Entomy commented 5 years ago

you could try rebasing as well!

Looks like GitHub just sees the new commits until the merge happens, as e5bf83f22f7398b52faff72d192f8dfc83142977 was added to the pull request. Would rebasing still be necessary?

psyomn commented 5 years ago

Could fix some of the issues, but no worries -- I can try and take care some of the conflicts. Let me know!

On a somewhat related note, a file that I can add that is a standard, but not universally supported .editorconfig can enforce some style guidelines in editors that support it. Linting (gnatpp probably?) is certainly a great idea. .editorconfig can help enforce these ahead of time; at least force me to be using spaces for this project. As you said you didn't want specific tooling, I'll add it to the .gitignore if you don't want it included.

I mean adding some flags for linting through the gpr file. I'm unfarmiliar with gnatpp, but if you're confident it can do a lot of the boring stuff without getting in the way, go ahead and add it (maybe in a separate PR)!

Entomy commented 5 years ago

Well I did fix spacing. I believe the only remaining issues are from code restructuring, which you'd have to resolve.in

Well like I said, adding linting flags is still a good idea, especially as that's the only way to handle naming/casing conventions. As far as I know GPR linting just calls gnatpp under the hood. Ignore the .editorconfig stuff, linting will take care of style rules.