stylebot icon indicating copy to clipboard operation
stylebot copied to clipboard

[Feature] Popout editor

Open Obsidianninja11 opened this issue 1 year ago • 1 comments

Is your feature request related to a problem? Please describe.

Sometimes the editor blocks part of the screen, or I need to pause a tab in debugger (calling debugger in the console), but then I can't add css with stylebot easily while the tab is paused.

Describe the solution you'd like

Add an option to dock the editor popped out in a new window (like in devtools).

Additional context

Images

Where the option could be: image

The option in devtools: image

Obsidianninja11 avatar Jun 15 '24 20:06 Obsidianninja11

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot avatar May 28 '24 13:05 rustbot

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

  • eea4c36ef2927f97c5f2b9b92da3490c61d29f35

rustbot avatar May 28 '24 13:05 rustbot

I'm not sure if there are conflicting opinions about this being constrained to modules or if that comment just thought it was more about not repeating the module name in the test name.

Personally, I can't think of any good reason even if the test fn isn't in a test mod. It's clear it's a test since we can see the macro annotation directly above it. Same with looking at the stdout from running cargo test.

andrewbanchich avatar May 30 '24 06:05 andrewbanchich

Sometimes you can't remove the test_* prefix because it's testing a function with the exact same name (1 2), so you'd end up with two functions of the same name, generating an error. You can work around it by adding a "_works" suffix like cargo's default-generated lib crate, but that feels like it's just working around the lint unnecessarily. I don't know how common this is but I expect quite a few FP reports from that.

(To be clear, this is an argument for at the very least making linting outside of test modules configurable. In test modules this seems like a non issue)

y21 avatar May 30 '24 10:05 y21

@y21 True, but the intention of this is to avoid a common Java-ism where people prefix test_ even when the test already is named something clear, like test_foo_fails_on_nonutf8_input. It's a common pattern we (and evidently others) have seen, and for those cases the assertion is already super clear.

I agree, though, that it's hard to think of a name for happy path tests, since it's either test_foo or foo_works. But then I've never seen foo_fails_on_nonutf8_input_works before, since that would make no sense.

So based on what I've seen:

  • The test_ prefix is commonly used for both happy and unhappy path testing.
  • It seems reasonable for happy path testing since the assertion is basically "yeah, it works".
  • Never seems reasonable for unhappy path testing since you should have clear assertions.
  • _works seems better since it means "happy path", so smaller chance for abuse since if you have a clear assertion it wouldn't make sense.

That being said, people can still write a test like foo_works and put negative assertions in it, but seems better than nothing 😅.

andrewbanchich avatar May 30 '24 11:05 andrewbanchich

That being said, I wonder why module_name_repetitions doesn't already catch these for functions in test modules. Maybe would be good to have it do that too, since it seems like we're talking about two different, but related, issues?

andrewbanchich avatar May 30 '24 11:05 andrewbanchich

@rustbot author

xFrednet avatar Jun 04 '24 12:06 xFrednet

:umbrella: The latest upstream changes (presumably #12849) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Jun 11 '24 23:06 bors

Hey @Caylub, this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?

If you have any questions, you're always welcome to ask them in this PR or on Zulip.

@rustbot author

xFrednet avatar Jun 30 '24 20:06 xFrednet

Hey this is triage, I'm closing this due to inactivity. If you want to continue this implementation, you're welcome to create a new PR. Thank you for the time, you already put into this!

Interested parties are welcome to pick this implementation up as well :)

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review

xFrednet avatar Jul 23 '24 06:07 xFrednet

The followup PR: #13710

farazdagi avatar Nov 19 '24 21:11 farazdagi