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

Improve call chain #4153

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Improve call chain #4153

wants to merge 7 commits into from

Conversation

Booome
Copy link

@Booome Booome commented Jan 13, 2024

Fixes #571

@JelleZijlstra JelleZijlstra self-requested a review January 13, 2024 16:17
Copy link

github-actions bot commented Jan 13, 2024

diff-shades results comparing this PR (8009888) to main (b7c3a9f). The full diff is available in the logs under the "Generate HTML diff report" step.

╭──────────────────────────── Summary ─────────────────────────────╮
│ 17 projects & 554 files changed / 18 048 changes [+10 537/-7511] │
│                                                                  │
│ ... out of 2 543 597 lines, 12 138 files & 23 projects           │
╰──────────────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

@tungol
Copy link
Contributor

tungol commented Jan 14, 2024

Seems like a lot of the diff in diff shades only have one method call. This in particular seems especially bad:

     assert (
         makeFY5253NearestEndMonthQuarter(
             weekday=1, startingMonth=3, qtr_with_extra_week=3
-        ).freqstr
+        )
+        .freqstr
         == "REQ-N-MAR-TUE-3"
     )

for comment in comment_line.strip(_COMMENT_PREFIX).split(
_COMMENT_LIST_SEPARATOR
)
for comment in comment_line.strip(_COMMENT_PREFIX)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels worse than the previous formatting.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't like the new one, I personaly like is:

                for comment in (
                    comment_line.strip(_COMMENT_PREFIX).split(_COMMENT_LIST_SEPARATOR)
                )

anyway, I will revert this change.

if delimiter_priority == DOT_PRIORITY:
if bt.delimiter_count_with_priority(delimiter_priority) == 1:
raise CannotSplit("Splitting a single attribute from its owner looks wrong")
if (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to keep this one as is, see the comment on the PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not quite sure which COMMENT you're referring to?

@JelleZijlstra
Copy link
Collaborator

Looking at the diff-shades output, a lot of changes feel like mild negatives, e.g.

         self.assertTrue(
-            logger.messages[0].startswith(
+            logger.messages[0]
+            .startswith(
                 "route matched for url http://localhost:8080"
                 "/archives/action1/article1; "
                 "route_name: 'foo', "
                 "path_info: "
             )
-        assert repr_locals.lines[0].startswith(
-            "x          = <[NotImplementedError() raised in repr()] ObjWithErrorInRepr"
+        assert (
+            repr_locals.lines[0]
+            .startswith(
+                "x          = <[NotImplementedError() raised in repr()]"
+                " ObjWithErrorInRepr"
+            )
         )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange formatting of fluent interface
3 participants