-
Notifications
You must be signed in to change notification settings - Fork 66
fix: Disable actions on autogenerated docs/nodes #2766
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,7 +200,10 @@ def perform(self) -> Optional[CreateOrUpdateNodeResult]: | |
| requirement.ordered_fields_lookup.clear() | ||
|
|
||
| self.populate_node_fields_from_form_object( | ||
| requirement, form_object, map_form_to_requirement_fields | ||
| requirement, | ||
| form_object, | ||
| map_form_to_requirement_fields, | ||
| existing_node_fields, | ||
| ) | ||
|
|
||
| # Updating Traceability Index: UID. | ||
|
|
@@ -298,7 +301,7 @@ def perform(self) -> Optional[CreateOrUpdateNodeResult]: | |
| parent.section_contents.insert(insert_to_idx, requirement) | ||
|
|
||
| self.populate_node_fields_from_form_object( | ||
| requirement, form_object, map_form_to_requirement_fields | ||
| requirement, form_object, map_form_to_requirement_fields, [] | ||
| ) | ||
| traceability_index.create_requirement(requirement=requirement) | ||
|
|
||
|
|
@@ -480,7 +483,17 @@ def populate_node_fields_from_form_object( | |
| map_form_to_requirement_fields: Dict[ | ||
| RequirementFormField, Optional[FreeTextContainer] | ||
| ], | ||
| existing_node_fields: Optional[List[SDocNodeField]] = None, | ||
| ) -> None: | ||
| if existing_node_fields is None: | ||
| existing_node_fields = [] | ||
|
|
||
| existing_fields_lookup: Dict[str, List[SDocNodeField]] = {} | ||
| for f in existing_node_fields: | ||
| if f.field_name not in existing_fields_lookup: | ||
| existing_fields_lookup[f.field_name] = [] | ||
| existing_fields_lookup[f.field_name].append(f) | ||
|
|
||
| # FIXME: Leave only one method based on set_field_value(). | ||
| for form_field_name, form_fields in form_object.fields.items(): | ||
| for form_field_index, form_field in enumerate(form_fields): | ||
|
|
@@ -490,34 +503,51 @@ def populate_node_fields_from_form_object( | |
| form_field_index=form_field_index, | ||
| value=form_field.field_value, | ||
| ) | ||
| continue | ||
|
|
||
| free_text_content: Optional[FreeTextContainer] = ( | ||
| map_form_to_requirement_fields[form_field] | ||
| ) | ||
| requirement_field: Optional[SDocNodeField] = ( | ||
| SDocNodeField( | ||
| node, | ||
| else: | ||
| free_text_content: Optional[FreeTextContainer] = ( | ||
| map_form_to_requirement_fields[form_field] | ||
| ) | ||
| requirement_field: Optional[SDocNodeField] = ( | ||
| SDocNodeField( | ||
| node, | ||
| field_name=form_field_name, | ||
| parts=free_text_content.parts, | ||
| multiline__="true" | ||
| if form_field.field_type | ||
| == RequirementFormFieldType.MULTILINE | ||
| else None, | ||
| ) | ||
| if free_text_content is not None | ||
| else None | ||
| ) | ||
| node.set_field_value( | ||
| field_name=form_field_name, | ||
| parts=free_text_content.parts, | ||
| multiline__="true" | ||
| if form_field.field_type | ||
| == RequirementFormFieldType.MULTILINE | ||
| else None, | ||
| form_field_index=form_field_index, | ||
| value=requirement_field, | ||
| ) | ||
| if free_text_content is not None | ||
| else None | ||
| ) | ||
| node.set_field_value( | ||
| field_name=form_field_name, | ||
| form_field_index=form_field_index, | ||
| value=requirement_field, | ||
| ) | ||
| if ( | ||
| requirement_field is not None | ||
| and free_text_content is not None | ||
| ): | ||
| for part_ in requirement_field.parts: | ||
| if isinstance(part_, str): | ||
| continue | ||
| part_.parent = requirement_field | ||
| if ( | ||
| requirement_field is not None | ||
| and free_text_content is not None | ||
| ): | ||
| for part_ in requirement_field.parts: | ||
| if isinstance(part_, str): | ||
| continue | ||
| part_.parent = requirement_field | ||
|
|
||
| # Preserve the existing origin metadata as is was not passed round-trip via the form. | ||
| new_field_list = node.ordered_fields_lookup.get(form_field_name) | ||
| if new_field_list and form_field_index < len(new_field_list): | ||
| new_field = new_field_list[form_field_index] | ||
|
|
||
| # Find the corresponding old field (if it existed) | ||
| if ( | ||
| form_field_name in existing_fields_lookup | ||
| and form_field_index | ||
| < len(existing_fields_lookup[form_field_name]) | ||
| ): | ||
| old_field = existing_fields_lookup[form_field_name][ | ||
| form_field_index | ||
| ] | ||
|
|
||
| if hasattr(old_field, "origin"): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR had a few instances of this. @thseiler I am curious if this is produced by an AI tool. Which tool did you possibly use to create it? I would be curious if we could extend our Dev Guide and let all AI tools avoid creating this pattern in the future. (I have been using Codex until now and haven't seen this pattern from it)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code was proposed by Gemini 3.1 Pro when it was reviewing my changes. It insisted that SDocDocument/SDocNodes were not just DOM objects but Abstract Syntax Tree objects (from the textX parser). It proposed duck typing to keep code decoupled from grammar, allowing for easier future evolution of the grammar. I have to admit I don't know enough about textX and DSL design and their best practices, but the argument seemed plausible... Thanks for the tip, I will give Codex a test drive... |
||
| new_field.origin = old_field.origin | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thseiler I started rebasing your branch and it needed a few changes that led me to editing your commit directly instead of push an additional code review one. The summary of my changes:
is_managed_by_source_code. One pattern here I tried to avoid is to add global logic to Jinja templates as that contradicts to the currently adopted pattern of routing everything through theview objectobjects.I am still reviewing the impact and consistency of your other changes, expecially the
file_traceability_index.andupdate_requirement.py. Should be done by the end of this weekend.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, @mettta will review the CSS styles to ensure that this change is consistent with everything else.