racket / drracket

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

Document drracket:comment-delimiters #635

Closed greghendershott closed 10 months ago

greghendershott commented 10 months ago

The documentation portion of #634.

Note that this commit does /not/ add to the documented list of keys read by Dr Racket, because this commit does not add the behavior of Dr Dracket doing that and using the comment styles to drive its (un)comment commands.

rfindler commented 10 months ago

I think it probably makes sense to clarify how newlines are going to be treated. Right now it reads like the empty string is a special case for newline handling. Is that right? I am realizing now that I may have misunderstood a detail from the previous discussion. If we're expecting tools to (for now at least) always put these in at the start and end of lines then we would not expect newlines to be inserted but instead the closing one to be inserted at the end of each line, right before the newline. If that's the case, then it probably helps to explain that in the docs so people understand better what characters to put in there. If it isn't the case (ie these are more general) then we might explain how they are going to be used but point out that they might be used in a more general way in the future.

Also, do we want to include some examples?

greghendershott commented 10 months ago

I think it probably makes sense to clarify how newlines are going to be treated. Right now it reads like the empty string is a special case for newline handling. Is that right?

Yes.

I am realizing now that I may have misunderstood a detail from the previous discussion. If we're expecting tools to (for now at least) always put these in at the start and end of lines then we would not expect newlines to be inserted but instead the closing one to be inserted at the end of each line, right before the newline. If that's the case, then it probably helps to explain that in the docs so people understand better what characters to put in there. If it isn't the case (ie these are more general) then we might explain how they are going to be used but point out that they might be used in a more general way in the future.

Also, do we want to include some examples?

I thought the default value was a good enough example, but I can add more.

greghendershott commented 10 months ago

p.s. I think the trickiest case is when both of the following are true:

  1. A lang supports region comments.

  2. A tool supports partial-line selections for its comment commands (doesn't round up to entire lines).

Now the tool needs to deal with stuff like nested comments, and selections spanning already-commented and not-commented spans. Probably it must use color-lexer tokens to get it right.

But if DrR sticks with entire-line selections for comment commands, then I think even region comment delimiters are straightforward. Comment always adds a start to BOL and an end before EOL. Un-comment always peels off the outermost pair if any.

However if that's not quite true, then DrR can just... ignore region comment delimiters entirely. Only use line comment delimiters. That would be no worse than today. It would be mildly better because at least it would use the line comment start supplied by each lang.

TL;DR I think the info key proposal is about supplying the info only the lang knows. Then it's up to each tool (its author and users) what or how much to do with that information. More isn't necessarily better, definitely not obligatory.

rfindler commented 10 months ago

Okay, this is all making sense to me. Thanks for the slow (repeated!) explanation. Also, it looks like Sublime Text also rounds-up when doing line comments. (I wonder if Emacs has a configuration parameter to get that behavior?)

Also, would it be clearer if we used two element lists to mean line comments and three element lists to be region comments? That way the empty string might be less confusing to the documentation reader?

sorawee commented 10 months ago

Would it be a good idea to tag the "style" (inline vs region) explicitly in the list? I'm thinking about forward compatibility where we might want to extend the list even longer to accommodate e.g. whether nested region comment is supported, and if we use the list length as a way to distinguish style, it might be more difficult to do this extension.

greghendershott commented 10 months ago

@rfindler @sorawee I think that sounds good. To make sure I understand, were you thinking something like:

(or/c (list 'line start padding)
      (list 'region start end padding))

?

And, I could add a bit in the doc to summarize some of what's in the thread above about line vs. region comments, as well as tool choices?

rfindler commented 10 months ago

yes, this sounds great;

And, I could add a bit in the doc to summarize some of what's in the thread above about line vs. region comments, as well as tool choices?

Especially this part!

greghendershott commented 10 months ago

I just pushed a commit trying to incorporate this feedback. Please let me know if this seems better, and any corrections/suggestions.

rfindler commented 10 months ago

It would be nice to include contracts to make things a bit more precise. Maybe where it says "list of comment styles" it can instead have the contract there? (That's kinds of far away from where they are discussed; maybe repetition is okay or maybe it is better to do a different change.)

I was inspired to think of this by wondering if continue should allow #f but I'm thinking that the empty string would do the same thing as false would so maybe that's not helpful?

rfindler commented 10 months ago

This looks great to me. I can squash and rebase if we're ready.

greghendershott commented 10 months ago

On the one hand there might still be some error/omission that will turn up through active dog-fooding. (Which I'm doing; I was using racket-hash-lang-mode in Emacs to edit lang-tools.scrbl. :smile: Also I plan to merge that to Racket Mode's main branch pretty soon, which possibly means more people trying it.)

On the other hand, we're not close to a release, so it would probably be OK if this is merged now, and another commit ends up tweaking it in (say) a few days/weeks?

rfindler commented 10 months ago

Okay, I've squashed and pushed to master here.