RFC: Adding Resyntax code reviews to core Racket repositories

Hello all! I've been developing Resyntax for some time now, and I've set it up to perform code reviews in the Typed Racket, Scribble, and DrRacket repositories. This has gone well thus far, so I'd like to add Resyntax code reviews to all of the core Racket repositories. This post is a request for maintainers of repositories in the racket GitHub organization to weigh in on this matter. Questions and suggestions welcome.

2 Likes

I looked a the repository you linked to, and found the line
@bold{This tool is extremely experimental.} Do not attempt to incorporate it into your projects yet.
in the main.scrbl file.

This line makes it seem as if you do not beieve your
project is ready for serious use. Is it ready enough
for you to remove this line?

Just how do you propose to add these Resyntax code reviews
to these repositories? As issues? Or by some other means?

-- hendrik

If resyntax is added to core Racket repositories, then, effectively, resynax becomes the official coding standard for Racket and I think the proposals it makes should be agreed upon by the Racket community.

For example, one of the refactorings it proposes is to replace for-each with for loops: resyntax/for-loop-shortcuts-test.rkt at master · jackfirth/resyntax · GitHub To me, this implies that the Racket community considers for-each deprecated. Is this the case?

There are a few other refactoring rules that I consider surprising (and I haven't looked at all of them), so it would be a good idea to have a discussion about which ones are considered official.

Alex.

2 Likes

@hendrikboom3 I looked a the repository you linked to, and found the line
@bold{This tool is extremely experimental.} Do not attempt to incorporate it into your projects yet.
in the main.scrbl file.

This line makes it seem as if you do not beieve your
project is ready for serious use. Is it ready enough
for you to remove this line?

Yes, I think it's time to remove that line. I no longer consider Resyntax experimental. Thanks for catching that :slight_smile:

@hendrikboom3 Just how do you propose to add these Resyntax code reviews
to these repositories? As issues? Or by some other means?

As a GitHub Action that automatically posts code reviews to pull requests opened against the repository. You can see an example here.

@alexh If resyntax is added to core Racket repositories, then, effectively, resynax becomes the official coding standard for Racket and I think the proposals it makes should be agreed upon by the Racket community.

I agree, and my intention is for Resyntax to reflect community consensus. Many of the rules in it - particularly the let to define rewrites - are based on the existing guidance in the Racket style guide.

@alexh For example, one of the refactorings it proposes is to replace for-each with for loops: resyntax/for-loop-shortcuts-test.rkt at master · jackfirth/resyntax · GitHub To me, this implies that the Racket community considers for-each deprecated. Is this the case?

No, I don't consider for-each deprecated and neither does Resyntax. Resyntax doesn't suggest rewriting all instances of for-each - just those that are obviously clearer when written with for. Currently, that includes for-each calls that pass multiline lambdas because rewriting those with for reduces indentation. A call like (for-each displayln strings) is left unchanged.

My understanding of the Racket community's position on for loops is that they're idiomatic and worth using when they make list processing code clearer, but that doesn't mean every case of map, filter, and foldl should be replaced with a for loop. I tried to capture that in Resyntax's recommendations, but please let me know if you find a case where the tool fell short.

@alexh There are a few other refactoring rules that I consider surprising (and I haven't looked at all of them), so it would be a good idea to have a discussion about which ones are considered official.

In general, if Resyntax suggests a rewrite that seems detrimental or overly noisy, just @ me in the comment thread. My intention is for the tool to only make suggestions that most Racketeers consider clear improvements or which the style guide already recommends. Also, I try to file issues for each rule I come up with and include real-world code I think the proposed rule would clear up, so the github repository issue list ought to serve as a record of the rationale behind each rule.

4 Likes

I'm super-psyched about Resyntax. I think that it ought to be the case—given Racket's focus on consistent syntax and thoughtful semantic decisions—that tooling like this is easier for us than it is for others. It's true that VSCode does some pretty nifty things, but their team is huge, and our team is tiny but amazing! Many kudos to @notjack .

5 Likes

I haven't had a lot of direct interaction with Resyntax, but I appreciate the careful thought you've put into the nuances of the rewriting suggestions, of which the for-each cases seem to be a microcosm: in addition to the size of the immediate lambda, I see you find cases like (string->list "hello") and (range 0 10) when for can avoid building an intermediate list.

It's particularly impressive given that Racket style is not exactly rigid: there's plenty of gnarly old code, and there are some subtler points of style that I think most of us would agree on when we notice them, but there's a great deal that comes down to judgement and taste. (Maybe some of that's to be expected from a language with no "reserved words" that tries to avoid "official" things having special magical powers and encourages things like #lang resyntax/testing/refactoring-test.) You've managed to design a tool that seems like a good fit for the Racket community. Everything I've seen from Resyntax leads me to look forward to helpful suggestions on my PRs, rather than dread some mindless autoformatter imposing mandates from afar.

3 Likes

If you want to reduce nesting, parendown works wonders. Parendown

I've always regretted that let is any more complicated than
(let name value stuff)
declaring name to have the value inside stuff.

With parendown, this makes for
(let n v
#/ let n2 vn
#/ cons n1 n2
)

and the like.

-- hendrik

2 Likes

It's interesting you mention VSCode, since one of my goals is to integrate Resyntax with the racket language server. That would let VSCode users see Resyntax suggestions directly in the editor.

4 Likes

Thank you very much for the kind words, I really appreciate it :slight_smile:

2 Likes

Resyntax is a very exciting project! I'd love for it to be configurable so that each project can turn certain rules on and off, similar to tools like Flake8. But the last time configurability came up, I believe the recommendation for adopting custom style conventions with Resyntax was to define a new #lang, e.g. #lang other-racket. This seems to suggest that Racket can only be written in one way, and any other way of writing it is a different language. I'm not sure if that's an accurate characterization and whether it is representative of views in the community. Also worth noting that Flake8 exists in a much more homogeneous ecosystem than Racket (i.e. python) and yet is configurable in this way. That general concern aside, as this is about official Racket repositories, that is of course up to the maintainers of those repositories and whether they agree with all of the rules. Consensus entails a lowest common denominator in many cases, which makes me think that there will be missed opportunities to make specialized suggestions that are favored in some repos but not others.

2 Likes

Does resyntax make suggestions about the new code or also about unmodified code? Sometimes there is a problem a PR is only one or two lines but the editor automatically fix the spaces in all the code.

Is resyntax a new check in the Actions in github? If resyntax makes a suggestion, does the PR get a red x?

1 Like

Does resyntax make suggestions about the new code or also about unmodified code? Sometimes there is a problem a PR is only one or two lines but the editor automatically fix the spaces in all the code.

Resyntax only analyzes modified lines. Lines not touched by a pull request are left alone.

Is resyntax a new check in the Actions in github? If resyntax makes a suggestion, does the PR get a red x?

Yes, it's a new check, and yes, the PR gets a red X. If Resyntax has suggestions it leaves a review on the pull request using the github code review system. The reviews look just like code reviews from other users, except the "user" is the github-actions bot.

Is it possible to avoid the red X? Some people get annoyed with it if the changes are not mandatory. Is it possible to use only the X and the check or it's also possibly to use the more neutral circle (or something like that)?.

1 Like

It's possible, but I'd prefer not to for now. If Resyntax is making suggestions that aren't worth paying attention to, I'd rather improve or disable those suggestions instead of encouraging people to ignore them. I want us all to collaborate on improving the Racket ecosystem using Resyntax. That's difficult to do if I don't get feedback on what Resyntax and the Racket community disagree on. If you see a suggestion that seems worthless, please let me know.

That said, the status check doesn't block submission, so you can always just ignore it anyway if you want to. Individual comments made by Resyntax can also be marked resolved manually without addressing them. That will hide them from everyone's view.

2 Likes

I used raco pkg install resyntax and have the module installed. The documentation https://docs.racket-lang.org/resyntax/cli.htmlsays that

Resyntax provides a command-line resyntax tool for analyzing and refactoring code. The tool has two commands: resyntax analyze for analyzing code without changing it, and resyntax fix for fixing code by applying Resyntax’s suggestions.

Where is this tool? It's not in my path and it's not under the /Applications/Racket_v8.5 directory.

I'm on macOS 10.15 and Racket 8.5.

1 Like

Double-check racket -e '(require setup/dirs)(displayln (find-user-console-bin-dir))'.

2 Likes

This got me too!

Use the Racket package manager to install in the installation scope.

raco pkg install --installation resyntax

The --installation flag (shorthand for --scope installation) installs packages for all users of a Racket installation and ensures resyntax is in your $PATH.

Best regards

Stephen :beetle:

3 Likes

Thank you, @benknoble and @spdegabrielle. Very helpful, both of you.

1 Like