racket / drracket

DrRacket, IDE for Racket
http://www.racket-lang.org/
Other
444 stars 93 forks source link

Run Resyntax on drracket/private #611

Closed jackfirth closed 1 year ago

jackfirth commented 1 year ago

This pull request runs Resyntax on all the reasonably-sized files in the drracket/private collection. It omits changes made to any of the huge files in this collection in order to make this pull request easier to review.

samth commented 1 year ago

Would it be possible for resyntax to print out a list of all the changes it made (something like "let->define, foo.rkt line 12")?

jackfirth commented 1 year ago

Yes, it currently does that in the command line. Unfortunately, that doesn't help much in this case because I only committed a subset of the changes. The full change report was this:

resyntax: --- summary ---

  Fixed 670 issues in 54 files.

  * Fixed 419 occurences of let-to-define
  * Fixed 55 occurences of cond-let-to-cond-define
  * Fixed 37 occurences of if-let-to-cond
  * Fixed 21 occurences of for-each-to-for
  * Fixed 21 occurences of define-lambda-to-define
  * Fixed 18 occurences of if-else-false-to-and
  * Fixed 12 occurences of if-begin-to-cond
  * Fixed 10 occurences of hash-ref-with-constant-lambda-to-hash-ref-without-lambda
  * Fixed 9 occurences of nested-for-to-for*
  * Fixed 8 occurences of hash-set!-ref-to-hash-update!
  * Fixed 7 occurences of de-morgan-or-to-and
  * Fixed 6 occurences of cond-else-cond-to-cond
  * Fixed 6 occurences of ormap-to-for/or
  * Fixed 6 occurences of nested-if-to-cond
  * Fixed 6 occurences of single-argument-plus-to-identity
  * Fixed 5 occurences of cond-else-if-to-cond
  * Fixed 5 occurences of de-morgan-and-to-or
  * Fixed 3 occurences of andmap-to-for/and
  * Fixed 3 occurences of sort-with-keyed-comparator-to-sort-by-key
  * Fixed 2 occurences of always-throwing-cond-to-when
  * Fixed 2 occurences of flat-contract-migration
  * Fixed 2 occurences of inverted-when
  * Fixed 2 occurences of define-case-lambda-to-define
  * Fixed 1 occurence of syntax-rearm-migration
  * Fixed 1 occurence of syntax-disarm-migration
  * Fixed 1 occurence of list->vector-for/list-to-for/vector
  * Fixed 1 occurence of equal-null-list-to-null-predicate
  * Fixed 1 occurence of let-values-then-call-to-call-with-values

One way we could surface this information better is by making Resyntax create multiple commits and group each change category into a single commit.

samth commented 1 year ago

One way we could surface this information better is by making Resyntax create multiple commits and group each change category into a single commit.

This sounds very nice.

rfindler commented 1 year ago

One way we could surface this information better is by making Resyntax create multiple commits and group each change category into a single commit.

This sounds very nice.

I agree!

rfindler commented 1 year ago

No, not always. Some of these examples would go wrong actually (for GC reasons).

But maybe if it only appears in the function position of applications (and never in other positions) the it should be a good suggestion. The things that are happening here are definitely very special cases and if they aren't being used like that, we can easily make a change to block resyntax's suggestion by adding that kind of reference.

It would be really great if resyntax noticed something that should be a private method and os currently a private field bound to a procedure!

Robby

On Tue, Mar 7, 2023 at 3:58 PM Jack Firth @.***> wrote:

@.**** commented on this pull request.

In drracket/drracket/private/module-language-tools.rkt https://github.com/racket/drracket/pull/611#discussion_r1128644060:

   (define/public (get-indentation-function) indentation-function)
  • (define range-indentation-function (λ (x y z) #f))
  • (define (range-indentation-function x y z) #f)

Perhaps Resyntax could rewrite (define (f x ...) body ...) to (define/private (f x ...) body ...) whenever it's within a class and f is never mutated? Would that always be correct?

— Reply to this email directly, view it on GitHub https://github.com/racket/drracket/pull/611#discussion_r1128644060, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBNMB4CETDUK5KP4JHFNDW26VRXANCNFSM6AAAAAAVPCXK2M . You are receiving this because your review was requested.Message ID: @.***>

jackfirth commented 1 year ago

@rfindler Could you file a feature request for that? https://github.com/jackfirth/resyntax

jackfirth commented 1 year ago

I'm closing this one in favor of #612.