search

racket / drracket

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

Re-provide build-trace% #361

Closed greghendershott closed 4 years ago

greghendershott commented 4 years ago

Although make-traversal is already provided, to use it you also need build-trace%. Provide officially so people don't need to do a naughty require of drracket/private/syncheck/traversals.

rfindler commented 4 years ago

I did not intend that function to be public. My thought is that you either make a new class that implements syncheck-annotations<%> (using annotations-mixin) or, if you just want the list all at once, you call show-content. Can you say why one of those two won't work for you?

rfindler commented 4 years ago

(And, perhaps more to the point, I'm not sure I want to be committed to backwards compatibility for that function the same that I am with the rest of the API.)

greghendershott commented 4 years ago

Can you say why one of those two won't work for you?

https://github.com/greghendershott/racket-mode/blob/master/racket/commands/check-syntax.rkt#L63-L96

greghendershott commented 4 years ago

If someone wanted to write something similar to the implementation of show-content, but slightly different (see previous comment, example why) all the pieces are provided including the make-traversal function and the current-annotations parameter... but just not the build-trace% class. I needed that missing piece. I assumed it was just an oversight.

rfindler commented 4 years ago

I think you're saying that you would like to be able to call a function that's just like show-content except it doesn't call expand? I've pushed something to help with that. (While I agree it is suboptimal to call expand a section time, I don't think it is inefficient.)

greghendershott commented 4 years ago

(While I agree it is suboptimal to call expand a section time, I don't think it is inefficient.)

I think you mean calling expand on syntax that's already fully expanded, isn't a big deal, and I agree.

(It is definitely a big deal to fully-expand, from scratch, more than necessary. When "check-syntax takes many seconds to complete", in reality check-syntax itself is pretty fast; the "many seconds" is the expansion time.)

When I was using module->imports I thought I learned that it was important to use the same namespace as was used for expansion. Maybe I was mistaken; I was juggling a lot at the time. But I'm a little concerned that using this flag will use a fresh make-base-namespace?

Racket Mode needs to support older/current/future versions of Racket. The #:fully-expanded? approach will be a little awkward to juggle for old/new versions. (I'd need to attempt to use the keyword arg, catch an error, assume an older version, yada yada.)

From what you said before:

My thought is that you either make a new class that implements syncheck-annotations<%> (using annotations-mixin) or, if you just want the list all at once, you call show-content.

It sounds like my better choice is the first one: I should copy-paste build-trace% ("make a new class") and supply that to make-traversal. That will work the same way with old/current/future versions.

rfindler commented 4 years ago

I agree with your conclusion, although you may find that build-trace% is idiosyncratic, so you might end up changing it too.

greghendershott commented 4 years ago

#:fully-expanded? seems useful for tools that can target Racket 7.7+.

you may find that build-trace% is idiosyncratic, so you might end up changing it too.

I understand and I did have a couple ideas how I might. Meanwhile it's good to have a baseline that works the same as before, without doing a naughty private/ require.

Thanks for discussing!