Gui reprocessing: update variable list from file content

[tmichela]

After testing #304 it was a bit unpractical to use for mainly 2 reasons:

• people often modify the context file with other editors
• the list would still contain variables present in the table, but not in the file.

So with this PR, we check the context file when we open the reprocess dialog.
It takes a few seconds in practice, but is loading only if we detect that the context file was modified. We could later add that saving the file from the GUI would also update that list.
I additionaly added a filter for the variable list which helps a lot when the context file contains many vars.

[codecov]

Codecov Report

Attention: Patch coverage is 31.37255% with 70 lines in your changes missing coverage. Please review.

Project coverage is 73.63%. Comparing base (c39468a) to head (98eafdf).

Files	Patch %	Lines
damnit/gui/process.py	29.59%	69 Missing ⚠️
damnit/gui/main_window.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##           feat/update-vars-on-save     #307      +/-   ##
============================================================
- Coverage                     74.37%   73.63%   -0.75%     
============================================================
  Files                            32       32              
  Lines                          4867     4953      +86     
============================================================
+ Hits                           3620     3647      +27     
- Misses                         1247     1306      +59     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tmichela]

I now parse the context file ast instead of executing it, which is much faster (a few ms).

[JamesWrigley]

I'm kinda against parsing the context file because at some point I'd like it to be possible to dynamically create variables, which would require runtime side-effects that wouldn't show up in the AST.

[tmichela]

can re reconsider that point once the feature exists?
It would be easy to change that back, and the benefit of not waiting 20 seconds for a list of variables here outweights a future rework, IMHO.

[JamesWrigley]

Ok, sure 👍

[takluyver]

The searchable list is very nice, and superqt looks like a good find. 👍

Honestly, I'm not keen on the rest of this PR. Yes, we want to deal with changes saved outside the GUI. But doing this specifically for the reprocess dialog means we've got two different ways of reading the context file (AST vs. execution), two different places in the GUI we're keeping a list of variables, and two different ways of ordering them for display. This all sounds like a recipe for confusing bugs and inconsistencies down the line.

How urgently do we need to make a change? We could obviously put it in as a shortcut and integrate it better later, but I feel like I spend a lot of time on DAMNIT trying to clean up shortcuts, and it's harder to get code review for refactoring compared to new features, so I'm not exactly enthusiastic about this.

Would you be happy if I try to work on making the GUI more broadly aware of the context file being edited elsewhere, with a view to getting something ready by the end of this week?

[tmichela]

How urgently do we need to make a change?

It is long requested feature and very much wanted.

I think I disagree with your view on this. Yes, it's 2 different way to read the file, but for 2 different use cases and it seems to me more complex to try to unify them than speciale case it. If you feel that strong about it, please have a go at it. But as I mentioned it already, the benefit of not waiting 20 seconds to update a list of variables is a good enough reason to me, in term of user experience, to special case this feature.

[takluyver]

I agree doing it faster is always nice, but if we decide parsing the AST is good enough, why keep a completely separate pathway to update the variable list by running the context file in a separate process? Using two separate mechanisms means we can't fully exploit either the speed advantage of a statically parsable context file, or the dynamism of an always-executed file.

[takluyver]

I'll take a look at my idea in any case. I think #291 was the bit I was wanting to get in before going further on that path, and that's merged now.

[tmichela]

if we decide parsing the AST is good enough, why keep a completely separate pathway to update the variable list by running the context file in a separate process?

I believe that the reason for running the context file is not to update the variable list, but rather to check for potential errors in the code preventing the context to be executed.

Using two separate mechanisms means we can't fully exploit either the speed advantage of a statically parsable context file, or the dynamism of an always-executed file.

I'm not sure to get what you mean here, but also why wouldn't it be possible to exploit both?
In this specific case we only care about listing variable objects anyway, so that's a pretty simple thing to solve.

[takluyver]

Sorry, I guess I got too heated about this. I'll step back and let someone else review it; please disregard my comments.