Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate template breakpoint targets #213

Closed
karthiknadig opened this issue Feb 14, 2019 · 5 comments
Closed

Validate template breakpoint targets #213

karthiknadig opened this issue Feb 14, 2019 · 5 comments
Labels
enhancement New feature or request

Comments

@karthiknadig
Copy link
Member

No description provided.

@int19h int19h transferred this issue from microsoft/ptvsd May 4, 2020
@int19h int19h added the enhancement New feature or request label Jun 19, 2020
@fabioz
Copy link
Collaborator

fabioz commented Sep 24, 2021

I started to take a look at it for django and I've hit a kind of roadblock:

I've gone up to the point where it's possible to collect the lines from a django template, but to actually create a template (i.e.: instantiate template.Template(template_contents)), django itself needs to be up and running, but at the point where breakpoints are added, that's not the case and trying to use the django parser for the template will fail saying that apps are still not configured (unfortunately django has a lot of global state and such global state can affect the template parsing).

The solutions I see are:

  1. manually parse it.
    • I don't like this approach much as keeping it up to date with what django sees in runtime as correct may be difficult, so, I see this as a brittle solution.
  2. delay the analysis to run when the template is actually already instantiated by django
    • i.e.: when we detect the template is about to be used the first time we'd collect the valid lines and validate if breakpoints are correct.
    • With this approach it should be possible to use the same mapping which we use to make a breakpoint hit, so, it should be consistent.
    • Unfortunately I don't see a way in the DAP to do such a delay in the notifications up to a point where the breakpoint could actually be validated (so, a proper implementation would need changes in the DAP itself). Given that, initially the idea would be just sending output messages to the console in case some breakpoint is not valid.

@int19h @karthiknadig I'm thinking about going with approach 2 -- do you think this is reasonable?

@int19h
Copy link
Contributor

int19h commented Sep 27, 2021

I think this is what the "breakpoint" event in DAP is supposed to be for. Specifically reporting a change for the breakpoint, and then when the client asks, reporting "verified" as true.

@fabioz
Copy link
Collaborator

fabioz commented Sep 30, 2021

I think this is what the "breakpoint" event in DAP is supposed to be for. Specifically reporting a change for the breakpoint, and then when the client asks, reporting "verified" as true.

Thanks for pointing that out. I'll see how to use that then.

@fabioz
Copy link
Collaborator

fabioz commented Oct 14, 2021

@int19h @karthiknadig @luabud

Using the breakpoint event does work (I still have some additional work to do in case breakpoints are added after the initial validation so that they don't appear disabled as we've loaded the template once already, but the structure to notify about changes in breakpoints is in place already).

Still, I was thinking, in the regular case, we actually change the breakpoint line if it was added to a wrong line... should we do that here too instead of notifying the user about the wrong line?

Also, another question: I've implemented it such that initially breakpoints will appear disabled with a message:

image

And then when the template is actually run it'll become enabled.

Is that OK or should they appear enabled and then be disabled when the template code is actually run?

@karthiknadig
Copy link
Member Author

If we have a way to move it to the correct line then I recommend doing so, since we already do it for python code.

Appearing disabled and then become enabled is the way other languages do it too. So I think this is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants