Skip to content
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

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

Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Mar 20, 2024

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 added bug Something isn't working formatter Related to the formatter labels Mar 20, 2024
Copy link

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

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
17 checks passed
@MichaReiser MichaReiser deleted the subscript-trailing-end-of-line-comment-left-bracket branch March 20, 2024 17:12
MichaReiser added a commit that referenced this pull request Mar 20, 2024
## 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
Labels
bug Something isn't working formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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