Add Python import tracking#41
Conversation
Mainly copied from the original Oneil
TimWhiting
left a comment
TimWhiting
left a comment
There was a problem hiding this comment.
Looks good to me other than my bits of feedback. A lot of churn with the diagnostic change, which might have been better as a separate PR.
| let (result, model_errors, expr_errors) = | ||
| runtime.eval_model_and_expressions(file, eval_expressions); | ||
|
|
||
| let mut model_saw_error_diagnostic = false; |
There was a problem hiding this comment.
This pattern is happening a lot, can we extract the error handling into a helper function?
There was a problem hiding this comment.
Yeah, that would make sense. I'll add that in.
There was a problem hiding this comment.
Did you split this file into smaller ones on purpose? Any reason for doing so? I would prefer to see fewer changes in a PR unless they are truly needed.
There was a problem hiding this comment.
Yeah, once a file gets too big for me to navigate comfortably, I tend to split it up into smaller files. It speeds up development for me since I'm able to more effectively narrow down where I'm looking for something and I don't have to filter through as much content in a file.
Basically, it helps me to find things when code organization uses the file system rather than just the linear structure of text.
It makes sense to reduce the amount of file splitting in a PR, though. It can be hard to identify what is actually new content. Would it be better to submit a separate PR splitting a file before working with the split file? Also, do you believe that splitting this file is unnecessary?
There was a problem hiding this comment.
This file is a bunch of object boilerplate. I would keep it together. I would split large files when they have implementation details concerning different objects that make it natural to split. If the objects are small and the functions are simple I would keep them together.
|
|
||
| ##### Fallback Calculations | ||
|
|
||
| Python functions may have dependencies that aren't always available, or may take a long time to run. You can specify a fallback calculation using the `?` operator. If the Python function fails (e.g., missing dependencies, runtime errors), Oneil will use the fallback and warn the user: |
There was a problem hiding this comment.
We should be clear that timeout is not currently supported, or support it with a flag.
There was a problem hiding this comment.
That's a good point. I can make that clear.
I agree. I started breaking up changes into smaller PRs with the three most recent PRs, but I didn't do it with Fallback support (submitted about a month ago). Most of your feedback was actually on code submitted for Fallback support, so I'll make the changes in that PR then rebase this and future PRs off of that. The only commits that are supposed to be for this PR are the last three commits. Alas, Github stacked PRs aren't publicly available yet... |
2 participants
NOTE: This PR is dependent on Fallback support and should not be reviewed until that PR is merged
This PR adds tracking for Python imports within a script by replacing the standard import mechanism (
builtins.__import__) with a custom mechanism that tracks local imports.