finishing touches to app execution finishes #34600
Conversation
dimagimon
added
the
reindex/migration
|
apologies, this is on my radar, but I still haven't gotten to it |
| inputs: dict | ||
| """Search inputs dict. Keys are field names and values are search values.""" |
There was a problem hiding this comment.
I haven't seen this pattern before - I assume these docstrings correspond to the vars above them? Is this an attrs pattern? A newer version of ":param inputs: blah blah"?
There was a problem hiding this comment.
It's not an attrs pattern, just something I've seen in other codebases. There was a PEP for it but it was rejected but still some ongoing discussion online: https://discuss.python.org/t/revisiting-attribute-docstrings/36413
There was a problem hiding this comment.
Oh neat, I see it's supported by Sphinx already. I also like the doc comment syntax - seems like it'd be super useful if we used autodoc more.
class Foo:
"""Docstring for class Foo."""
#: Doc comment for class attribute Foo.bar.
#: It can have multiple lines.
bar = 1
flox = 1.5 #: Doc comment for Foo.flox. One line only.
baz = 2
"""Docstring for class attribute Foo.baz."""
def __init__(self):
#: Doc comment for instance attribute qux.
self.qux = 3
self.spam = 4
"""Docstring for instance attribute spam."""| if self.validate_inputs: | ||
| # children will be executed instead of the parent | ||
| inputs = {} | ||
| for field, value in self.inputs.items(): |
There was a problem hiding this comment.
Is order significant here? Looks like it would be since the validation steps are sequential and include previous inputs. Might be worth documenting that somewhere
There was a problem hiding this comment.
I don't think order is relevant since search field aren't dependent on each other. This is cumulative because that's what the UI does, it always submits all inputs on every change so as you set more filter values the effect is cumulative.
| <div class="col col-sm-1 {% if status.success %}bg-success{% else %}bg-danger{% endif %}" style="width: 10px"> | ||
| <a href="{% url "app_execution:workflow_log" status.id %}" class="w-100 h-100 d-block"> </a> | ||
| </div> |
| @use_bootstrap5 | ||
| def workflow_log_list(request, pk): | ||
| logs = AppExecutionLog.objects.filter(workflow_id=pk).order_by("-started").all() | ||
| context = _get_context( |
There was a problem hiding this comment.
I don't think I paid much attention to this util when reviewing the previous PR. This is something I've long felt is a frustration with HQ code - that you essentially need to use a class-based view that inherits from like 8 others in order to have the context necessary to render the navbar, sidebar, and the like. I'd love to see a replicable pattern for avoiding that. An idea I had a while ago was to use a SiteContext object or some such that is inherited in the same way, but only deals with this context. I could also see a functional version that chains to parent contexts. Dunno, but I wanted to voice my support for what you're doing here.
There was a problem hiding this comment.
👍🏻 I feel the same and have to dig through a lot of code to figure out what goes in here and how it works.
Co-authored-by: Ethan Soergel <esoergel@users.noreply.github.com>
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Product Description
Rounding the edges on the app workflows code:
Workflow list
Create / Edit
Test workflow
Log list
Log details
Safety Assurance
Safety story
Admin only feature
Automated test coverage
Existing tests
QA Plan
None
Migrations
Rollback instructions
If the revert is permanent then the DB migration should be rolled back.
Labels & Review