Skip to content

Fix instable formatting for trailing subscribt end-of-line comment#10492

Merged
MichaReiser merged 1 commit intomainfrom
subscript-trailing-end-of-line-comment-left-bracket
Mar 20, 2024
Merged

Fix instable formatting for trailing subscribt end-of-line comment#10492
MichaReiser merged 1 commit intomainfrom
subscript-trailing-end-of-line-comment-left-bracket

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Mar 20, 2024 •
edited
Loading

Summary

This PR fixes an instability where formatting a subscribt
where the slice is not an ExprSlice and it has a trailing end-of-line comment after its opening [ required two formatting passes to be stable.

The fix is to associate the trailing end-of-line comment as dangling comment on [ to preserve its position, similar to how Ruff does it for other parenthesized expressions.
This also matches how trailing end-of-line subscript comments are handled when the slice is an ExprSlice.

Fixes #10355

Versioning

Shipping this as part of a patch release is fine because:

  • It fixes a stability issue
  • It doesn't impact already formatted code because Ruff would already have moved to the comment to the end of the line (instead of preserving it)

Test Plan

Added tests

@MichaReiser MichaReiser requested a review from konstin March 20, 2024 16:48
@MichaReiser MichaReiser added bug Something isn't working formatter Related to the formatter labels Mar 20, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2024

ruff-ecosystem results

Formatter (stable)

ℹ️ ecosystem check detected format changes. (+15 -5 lines in 3 files in 1 projects; 42 projects unchanged)

demisto/content (+15 -5 lines across 3 files) 

ruff format --no-preview --exclude Packs/ThreatQ/Integrations/ThreatQ/ThreatQ.py

Packs/ApiModules/Scripts/CoreIRApiModule/CoreIRApiModule_test.py~L770

 
     assert (
         "handle_outgoing_issue_closure Closing Remote incident incident_id=None with status resolved_security_testing"
-        in request_data_log.call_args[0][0]  # noqa: E501
+        in request_data_log.call_args[  # noqa: E501
+            0
+        ][0]
     )

Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py~L241

                 dist = {k: v * 100 / total for k, v in chosen.items()}
                 dist_total[cluster_number] = {}
                 dist_total[cluster_number]["number_samples"] = sum(
-                    self.clustering.raw_data[self.clustering.model.labels_ == cluster_number].label.isin(  # type: ignore  # type: ignore
+                    self.clustering.raw_data[  # type: ignore
+                        self.clustering.model.labels_ == cluster_number
+                    ].label.isin(  # type: ignore
                         list(chosen.keys())
                     )
                 )  # type: ignore

Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py~L603

             "pivot": "clusterId:" + str(cluster_number),
             "incidents_ids": [
                 x
-                for x in incidents_df[clustering.model.labels_ == cluster_number].id.values.tolist()  # type: ignore
+                for x in incidents_df[  # type: ignore
+                    clustering.model.labels_ == cluster_number
+                ].id.values.tolist()
             ],  # type: ignore
             "incidents": incidents_df[clustering.model.labels_ == cluster_number][  # type: ignore
                 display_fields + fields_for_clustering_remove_display

Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py~L617

     d_outliers = {
         "incidents_ids": [
             x
-            for x in incidents_df[clustering.model.labels_ == -1].id.values.tolist()  # type: ignore
+            for x in incidents_df[  # type: ignore
+                clustering.model.labels_ == -1
+            ].id.values.tolist()
         ],  # type: ignore
         "incidents": incidents_df[clustering.model.labels_ == -1][display_fields].to_json(  # type: ignore
             orient="records"

Packs/Orca/Integrations/Orca/Orca.py~L47

 
     def get_alerts_by_filter(
         self, alert_type: Optional[str] = None, asset_unique_id: Optional[str] = None, limit: int = 1000
-    ) -> Union[List[Dict[str, Any]], str]:  # pylint: disable=E1136 # noqa: E501  # pylint: disable=E1136 # noqa: E125
+    ) -> Union[  # pylint: disable=E1136 # noqa: E501
+        List[Dict[str, Any]], str
+    ]:  # pylint: disable=E1136 # noqa: E125
         demisto.info("get_alerts_by_filter, enter")
 
         url_suffix = "/alerts"

Formatter (preview)

ℹ️ ecosystem check detected format changes. (+15 -5 lines in 3 files in 1 projects; 42 projects unchanged)

demisto/content (+15 -5 lines across 3 files) 

ruff format --preview --exclude Packs/ThreatQ/Integrations/ThreatQ/ThreatQ.py

Packs/ApiModules/Scripts/CoreIRApiModule/CoreIRApiModule_test.py~L768

 
     assert (
         "handle_outgoing_issue_closure Closing Remote incident incident_id=None with status resolved_security_testing"
-        in request_data_log.call_args[0][0]  # noqa: E501
+        in request_data_log.call_args[  # noqa: E501
+            0
+        ][0]
     )

Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py~L241

                 dist = {k: v * 100 / total for k, v in chosen.items()}
                 dist_total[cluster_number] = {}
                 dist_total[cluster_number]["number_samples"] = sum(
-                    self.clustering.raw_data[self.clustering.model.labels_ == cluster_number].label.isin(  # type: ignore  # type: ignore
+                    self.clustering.raw_data[  # type: ignore
+                        self.clustering.model.labels_ == cluster_number
+                    ].label.isin(  # type: ignore
                         list(chosen.keys())
                     )
                 )  # type: ignore

Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py~L603

             "pivot": "clusterId:" + str(cluster_number),
             "incidents_ids": [
                 x
-                for x in incidents_df[clustering.model.labels_ == cluster_number].id.values.tolist()  # type: ignore
+                for x in incidents_df[  # type: ignore
+                    clustering.model.labels_ == cluster_number
+                ].id.values.tolist()
             ],  # type: ignore
             "incidents": incidents_df[clustering.model.labels_ == cluster_number][  # type: ignore
                 display_fields + fields_for_clustering_remove_display

Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py~L617

     d_outliers = {
         "incidents_ids": [
             x
-            for x in incidents_df[clustering.model.labels_ == -1].id.values.tolist()  # type: ignore
+            for x in incidents_df[  # type: ignore
+                clustering.model.labels_ == -1
+            ].id.values.tolist()
         ],  # type: ignore
         "incidents": incidents_df[clustering.model.labels_ == -1][display_fields].to_json(  # type: ignore
             orient="records"

Packs/Orca/Integrations/Orca/Orca.py~L47

 
     def get_alerts_by_filter(
         self, alert_type: Optional[str] = None, asset_unique_id: Optional[str] = None, limit: int = 1000
-    ) -> Union[List[Dict[str, Any]], str]:  # pylint: disable=E1136 # noqa: E501  # pylint: disable=E1136 # noqa: E125
+    ) -> Union[  # pylint: disable=E1136 # noqa: E501
+        List[Dict[str, Any]], str
+    ]:  # pylint: disable=E1136 # noqa: E125
         demisto.info("get_alerts_by_filter, enter")
 
         url_suffix = "/alerts"

@MichaReiser
Copy link
Member Author

MichaReiser commented Mar 20, 2024 •
edited
Loading

Hmm, I was wrong. I need to look into the ecosystem changes.

Never mind. These are unformatted projects and the ecosystem changes show that the comment position is now preserved

@MichaReiser MichaReiser merged commit 954a48b into main Mar 20, 2024
@MichaReiser MichaReiser deleted the subscript-trailing-end-of-line-comment-left-bracket branch March 20, 2024 17:12
@MichaReiser MichaReiser mentioned this pull request Mar 20, 2024
Merged
MichaReiser added a commit that referenced this pull request Mar 20, 2024
@MichaReiser
Fix subscript comment placement with parenthesized value (#10496)
9d705a4 
## Summary

This is a follow up on #10492 

I incorrectly assumed that `subscript.value.end()` always points past
the value. However, this isn't the case for parenthesized values where
the end "ends" before the parentheses.

## Test Plan

I added new tests for the parenthesized case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@konstin konstin konstin approved these changes

@charliermarsh charliermarsh Awaiting requested review from charliermarsh

Assignees

No one assigned

Labels

bug Something isn't working formatter Related to the formatter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

formatter: Two iterations required to converge on specific comment + slice case

2 participants

@MichaReiser @konstin