Write representer

[mcastorina]

I would like to work on the representer for GoLang https://github.com/exercism/go-representer

If this is already being worked on, feel free to close this issue.

Design thoughts:

• remove comments
• change variables to standardize the naming
• use go fmt

I am still new to Exercism development, so some help on the framework would be appreciated.

I also think https://staticcheck.io/ could be useful for automated suggestions or as a tool for mentors when reviewing the "common" cases.

[junedev]

@mcastorina Great that you want to work on the representer! Nobody is working on this yet and there is so much to do currently, we are happy if you can pick this up. (Sidenote: I will move this issue to the representer repo later.)

To avoid unpleasant surprises, be aware that while it would be great to have the representer ready to go, the frontend that goes with it (where mentors leave comments for a representation) is not fully implemented/ not live yet. I think it is definitely planned for this year but I can't say when it goes live exactly. Until then, representers are running in the background but we don't do anything with the output yet.

I am not super familiar with analyzers and representers myself yet, but we will figure things out together. And there are always other people to help us on Slack. You probably already found the general documentation here: https://exercism.org/docs/building/tooling/representers

Regarding the docker image for the representer, you can probably re-use the docker file etc from the analyzer as they both require the same setup: https://github.com/exercism/go-analyzer/

Regarding staticcheck: This belongs into the domain of the analyzer. We already have an issue there that we need to set up a replacement for golint https://github.com/exercism/go-analyzer/issues/16.

Regarding you design thoughts: Sounds very reasonable as a first iteration. Later on, we probably also need to think about things that go fmt does not pick up, e.g. it will not normalize whether there is an empty line between two statements or not.

[mcastorina]

What are your thoughts on using go/ast instead of go fmt for the first iteration? It would certainly make renaming identifiers easier. We can even output the AST in representation.txt I believe (according to the representer docs).

[junedev]

You are right, as you said, you probably need the AST parsing anyway so I would recommend starting with that. My gut feeling is that outputting human readable code (just with these standardized identifiers) would be the best choice for the representation file, e.g. because it readable and it could theoretically be parsed back into an AST easily which is not true for a "serialized AST" (e.g. like the one shown in the docs). But if you see advantages of doing something else, I am interested to hear them.

Go has a ready to use function to go from AST to "go fmt" formatted code, you can see this used in the test runner: https://github.com/exercism/go-test-runner/blob/main/testrunner/ast.go#L100 (That file in general might give some guidance regarding the AST parsing in case you need that.)

[junedev]

@mcastorina How is it going? Would be great if you could leave a quick note whether you are still working on this.

[mcastorina]

@junedev sorry for disappearing! I've been pretty busy these past months so I haven't made any progress. I'll work on it when I can, but it's probably best to unassign me and allow anyone else to pick up the issue.

[junedev]

No problem, thanks for getting back to me. I unassigned you for now and freed up the task again. We can still give it back to you later if you want it again at some point and no one else claimed it.

[tehsphinx]

@junedev I'll take a look into this. Get a first working version done, so it can then be iterated upon.

[junedev]

@tehsphinx Sounds great, thanks a lot!

[junedev]

@tehsphinx wrote in https://github.com/exercism/go-representer/pull/15#issuecomment-1011951758

About different variable definition, incrementing a variable, etc: I think it makes sense to comment on that on earlier exercises and later on not. However my suggestion would be to have the same representer logic for all exercises and rather handle that with the analyzer. In my experience the analyzer is much more exercise specific anyway, so I think it fits better.

I was also wondering whether these different declaration etc isn't something that the analyzer is better suited for. Not sure how it is for Go but in other analyzers I have seen general check functions that can be used in multiple exercises. So instead of letting mentors manually enter feedback for each of these variations, the analyzer would leave the feedback on those for some early exercises and the mentor that comments on the representation only needs to give feedback for one variant instead of all the different combinations of where different declarations could be used.

One other thought that crossed my mind: What happens/ should happen with the placeholders in case there is variable shadowing going on. Do they get the same placeholder name or a different one?

[tehsphinx]

@junedev 1) Ok, then I'll next move on to looking into this for the representer:

checking on how to generalize:

• variable declarations
• variable increment

Any others anyone can think of?

2) Different placeholders for different scopes

From the representer docs https://exercism.org/docs/building/tooling/representers/interface:

It is important to note that all identical names must be replaced with the same placeholder, irrespective of scope.

According to that they should have the same placeholder 🙂

[junedev]

According to that they should have the same placeholder

Ah ok, it might make sense to cover this special case with a test case at some point so we ensure we follow the docs here.

[junedev]

Any others anyone can think of?

• make without capacity and length vs. non make version (slices/maps)
• new(someStruct) vs. &someStruct{}

Would be good to keep a list of what normalizations we decided for in the README.

In general, I think Go is a rather thankful language to write a representer for as many things only have one way of writing them and gofmt already takes care of a lot.

[tehsphinx]

• order of functions, variable declarations, imports (?) on package level should not matter.
• distribution into multiple files should not matter

[junedev]

@tehsphinx Some thoughts on the points you mentioned.

• Order of functions: Can have a bit lower priority imo as many exercises only need the functions already defined in the stub and students rarely change the order here. As for custom helper functions, if they are used, it is a bit more unlikely that it will match a lot of other solutions representation wise.
• Import order: Usually solutions have no imports so even lower priority than function order imo.
• Multiple files: Are you sure it makes sense to normalize this? Most solutions will be written in just one file (additional files are mostly only used to provide predefined types etc. that the student should not modify). If a students splits their solution into multiple files, then I think there is a high likelihood that a mentor filling out the representer UI would comment on that, either in a positive ("Nice that you created types.go") or negative ("No need to create 1 file for each function.") way. Also here don't think the analyzer is a good fit to evaluate this. In conclusion my feeling is that file structure should not be normalized. I liked you old system with the file separator you had. Maybe the filenames should go into the placeholder map as well. :thinking:

[tehsphinx]

Yes, agree that all of these are quite low priority.

They only came up since I added a multi-file test. Then realized the tests were sometimes failing as I forgot to make sure they are processed in the same order every time (iteration over map). Next I realized that the names would dictate the order of and I can't use the file names anyway and would need placeholders instead. Since file distribution in a package does not matter in Go I looked into combining them. There is an AST function to combine all files of a package into one file (so that was easy) but the order in that file is a bit unusual (e.g. imports are not moved to the top). So... I think I'll just order the items in that file, too. Then it's clean and we can have a nice result for the multi-file test, too. 😄

[junedev]

Since the main task here (writing the representer) was done thanks to @tehsphinx, I will close this issue and create a new one for tracking all the ideas for additional normalizations.