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

change the full outpot view from link to button #3244

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

Conversation

AbdelbassitAb
Copy link

No description provided.

Copy link
Member

@vivekashok1221 vivekashok1221 left a comment

Choose a reason for hiding this comment

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

Hello! I haven't gone over everything yet but one of the tests is failing. I've left a few comments on how to fix them. Feel free to ask in the #dev-contrib channel (or here, if that's what you prefer) if you need any further clarification or help.

ctx.send.assert_called_once()
call_args = ctx.send.call_args
message_content = call_args.kwargs["content"]
Copy link
Member

@vivekashok1221 vivekashok1221 Jan 21, 2025

Choose a reason for hiding this comment

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

>       message_content = call_args.kwargs["content"]
E       KeyError: 'content'

tests/bot/exts/utils/snekbox/test_snekbox.py:423: KeyError

CI is failing because content appears to be passed to ctx.send as a positional argument rather than a keyword argument. I believe this should fix it:

Suggested change
message_content = call_args.kwargs["content"]
message_content = call_args.args[0]

"has completed with return code 0."
"\n\n```ansi\nWay too long beard\n```\nFull output: lookatmybeard.com"
message_content,
":white_check_mark: Your 3.12 eval job has completed with return code 0.\n\n```ansi\nToo long beard\n```",
Copy link
Member

Choose a reason for hiding this comment

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

The mock message and expected output are different.

Suggested change
":white_check_mark: Your 3.12 eval job has completed with return code 0.\n\n```ansi\nToo long beard\n```",
":white_check_mark: Your 3.12 eval job has completed with return code 0.\n\n```ansi\nWay too long beard\n```",

Comment on lines 433 to 444
button = next(
(item for item in view.children if isinstance(item, ui.Button)), None
)
self.assertIsNotNone(button)
self.assertEqual(button.label, "View Full Output")
self.assertEqual(button.url, "lookatmybeard.com")
Copy link
Member

Choose a reason for hiding this comment

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

Will button in this case be the "View Full Output" button or "Run in 13.3" button? (Run in 13.3 button appears first.)

Copy link
Author

@AbdelbassitAb AbdelbassitAb Jan 23, 2025

Choose a reason for hiding this comment

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

Check if the view contains the correct button

self.assertIsNotNone(view)
buttons = [item for item in view.children if isinstance(item, ui.Button)]
button = (
buttons[-1] if buttons else None
) # Get the last button if buttons exist`
what do you think about this ? @vivekashok1221 to check for the last button , which is always the View Full Output button

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this will work.

Copy link
Member

@ChrisLovering ChrisLovering left a comment

Choose a reason for hiding this comment

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

Can you please revert the formatting/linting changes that seem to have been done by black or similar which was given incorrect line length.

These changes are irrelevant to your PR, and makes reviewing more difficult that it needs to be.

@AbdelbassitAb
Copy link
Author

Hey @ChrisLovering , i cant commit because of that linting issue , that s why i changed it , how can i commit without fixing it ?

@ChrisLovering
Copy link
Member

Hey @ChrisLovering , i cant commit because of that linting issue , that s why i changed it , how can i commit without fixing it ?

Our pre-commit-config doesn't include a formatter that would make these changes.

I suspect that you have black/ruff setup on your editor to format on save, which is causing these changes. You will need to disable that for this repository.

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.

Use link buttons for showing full output, if truncated, of the eval command
3 participants