Automated Resyntax fixes #699
Conversation
![resyntax-ci[bot]](https://avatars.githubusercontent.com/in/999945?s=60&v=4){kind=small}
Keep imports in `require` sorted and grouped by phase, with collections before files.
This `if` expression can be refactored to an equivalent expression using `and`.
This definition is not used.
The `instantiate` form is for mixing positional and by-name constructor arguments. When no positional arguments are needed, use `new` instead.
Internal definitions are recommended instead of `let` expressions, to reduce nesting.
These nested `for` loops can be replaced by a single `for*` loop.
This `sort` expression can be replaced with a simpler, equivalent expression.
Internal definitions are recommended instead of `let` expressions, to reduce nesting.
The `instantiate` form is for mixing positional and by-name constructor arguments. When no positional arguments are needed, use `new` instead.
This `let-values` expression can be replaced with a simpler, equivalent `call-with-values` expression.
|
|
||
| (set! update-label | ||
| (λ (s) | ||
| (if (and s (not (null? s))) |
There was a problem hiding this comment.
this should have become a cond?
There was a problem hiding this comment.
The let* expression in the true branch can't be refactored into definitions because of the shadowing it's doing in the binding for lines, so Resyntax won't refactor this if into a cond. It only does that for ifs with lets when the let is refactorable.
There was a problem hiding this comment.
I'm not seeing a shadowing binding for lines. When I put lines just behind the λ, I get a free id error.
There was a problem hiding this comment.
Oh drat I misread this bit below:
I checked out the code and ran Resyntax over it manually and it did indeed fix this code, so I think the issue is it just ran up against the fix limit.
| (get-snip-hspace)) | ||
| this-major)))]))) | ||
| (define level (car levels)) | ||
| (car level) |
| [else | ||
| (set! last-size this-size) | ||
| (set! | ||
| last-name |
There was a problem hiding this comment.
this is probably a bad place to break a set!
|
There was a big change in the middle of standalone-module-browser.rkt that the diff wasn't very informative so I couldn't really be sure nothing is wrong. |
|
(but things look good to me overall) |
Does reviewing the change on a commit-by-commit basis help? It seems the large diff came as a result of some |
It looks like 8ad65d5 is just as non-easy to review as the whole thing, at least when I view it in github's ui. Is there a better way? |
|
@rfindler perhaps you are viewing the diff with the "Unified" layout? Can you switch to the "Split" layout and see if it helps? Below is how the "Split" layout displays to me, which is quite reasonable. 
|
That's what I see. Perhaps I'm too picky, but let me ask you: how many of the 125 lines starting with the one numbered 365 actually changed and how many are just indentation shifts? |
|
Ah, if you don't want to see reindentation, you can select "Hide whitespace". It looks like the GitHub UI is buggy, and the change will not take effect immediately. You need to additionally refresh the page once. After doing that, here's how it looks for me. 
|
|
Oh! Thanks! That works! (It sometimes shows the whitespace changes highlighted (like in the last line of your most recent screenshot) and I was missing those, but this works well. I'll try to remember to use that! |
better fix to racket#699 also related to racket/racket#5008
Resyntax fixed 20 issues in 4 files.
let-to-defineinstantiate-to-newcond-let-to-cond-definetidy-requirelet-values-then-call-to-call-with-valuesnested-for-to-for*unused-definitionsort-with-keyed-comparator-to-sort-by-keyif-else-false-to-and