racket / scribble

Other
194 stars 90 forks source link

Run Resyntax on manual-code.rkt #363

Closed jackfirth closed 1 year ago

jackfirth commented 1 year ago

This pull request runs Resyntax on one of the files in the core of Scribble.

mflatt commented 1 year ago

The only reason I didn't merge this right away is that the change from null to '() gives me pause. Can you say more about that change? I tend to write null because it's easier for me to read (since there are typically lots of parentheses and quotes serving various roles in my code already), even though I don't particularly like the name null.

jackfirth commented 1 year ago

I think it's weird how many different ways there are to write the empty list - null, '(), empty, and (list). I think we ought to standardize on one of them and I assumed '() is the one most racketeers would prefer, for these reasons:

I'm open to tweaking or disabling that rule though.

mflatt commented 1 year ago

I'm in favor of disabling this rule. While I appreciate the benefits of a more canonical form for the empty list, '() or null seem better to me depending on the context, and the variation doesn't seem like either a source of confusion or something that makes code more difficult to read and modify. That's unlike converting let to define, where I think there's widespread agreement that define makes code easier to read and modify.

jackfirth commented 1 year ago

Sounds good to me, opened https://github.com/jackfirth/resyntax/issues/181.

Could we also just... move empty from racket/list to racket/base? It's incredibly bizarre that the nice readable name for this value is something you have to import.

soegaard commented 1 year ago

Could we also just... move empty from racket/list to racket/base? It's incredibly bizarre that the nice readable name for this value is something you have to import.

I am almost certain that will cause problems for modules that use empty for other purposes.

For example in the various modules of pfds.

jackfirth commented 1 year ago

Sigh. You're likely right.

jackfirth commented 1 year ago

@mflatt I've updated the pull request not to include the empty list changes. I also updated Resyntax not to make that suggestion, but it seems the package server hasn't caught up with that change yet, so the comments are still showing up on this pull request.

mflatt commented 1 year ago

Thanks!