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

TYPE: fold the r# in raw-identifiers #3759

Merged
merged 3 commits into from Apr 28, 2019
Merged

TYPE: fold the r# in raw-identifiers #3759

merged 3 commits into from Apr 28, 2019

Conversation

@ice1000
Copy link
Contributor

@ice1000 ice1000 commented Apr 25, 2019

Signed-off-by: ice1000 ice1000kotlin@foxmail.com

Before:
Screenshot from 2019-04-25 11-15-32
After:
Screenshot from 2019-04-25 11-15-13

@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Apr 25, 2019

I did some general cleanup in related files.

@mchernyavsky mchernyavsky self-assigned this Apr 25, 2019
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Apr 25, 2019

I have received some negative comments on this feature. Maybe I should make it disabled by default (but how can I test it then?).

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@mchernyavsky
Copy link
Member

@mchernyavsky mchernyavsky commented Apr 26, 2019

I have received some negative comments on this feature. Maybe I should make it disabled by default (but how can I test it then?).

I'm not sure, but it seems that in tests all regions are folded (regardless of the settings).

@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Apr 26, 2019

@mchernyavsky I've modified the tests to let it:

  • being disabled by default
  • still testable because I've enabled it before running the specific test case

, please take a look.

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Apr 26, 2019

bors ping

@bors
Copy link
Contributor

@bors bors bot commented Apr 26, 2019

pong

@mchernyavsky
Copy link
Member

@mchernyavsky mchernyavsky commented Apr 28, 2019

LGTM, thanks!
bors r+

bors bot added a commit that referenced this issue Apr 28, 2019
3759: TYPE: fold the `r#` in raw-identifiers r=mchernyavsky a=ice1000

Signed-off-by: ice1000 <ice1000kotlin@foxmail.com>

<!--
Hello and thank you for the pull request!

We don't have any strict rules about pull requests, but you might check
https://github.com/intellij-rust/intellij-rust/blob/master/CONTRIBUTING.md
for some hints!

Note that we need an electronic CLA for contributions:
https://github.com/intellij-rust/intellij-rust/blob/master/CONTRIBUTING.md#cla

After you sign the CLA, please add your name to
https://github.com/intellij-rust/intellij-rust/blob/master/CONTRIBUTORS.txt

:)
-->

Before:
![Screenshot from 2019-04-25 11-15-32](https://user-images.githubusercontent.com/16398479/56747164-784c8680-674b-11e9-8bb4-9c99b5459cfc.png)
After:
![Screenshot from 2019-04-25 11-15-13](https://user-images.githubusercontent.com/16398479/56747171-7be00d80-674b-11e9-9835-994bb2e05f43.png)



Co-authored-by: ice1000 <ice1000kotlin@foxmail.com>
@bors
Copy link
Contributor

@bors bors bot commented Apr 28, 2019

@bors bors bot merged commit b6ddc48 into intellij-rust:master Apr 28, 2019
2 checks passed
@ice1000 ice1000 deleted the fold-raw-keywords branch Apr 28, 2019
ice1000
Copy link
Contributor

ice1000 commented on 8ddb6f9 Apr 28, 2019

Why this already-force-pushed commit is included in the repo? Is this a feature of bors?

ice1000
Copy link
Contributor

ice1000 commented on 8ddb6f9 Apr 28, 2019

Nevermind. I didn't --amend.
I'm so sorry about this.

@Undin
Copy link
Member

@Undin Undin commented Apr 29, 2019

Hm, I'm not sure that it's a useful feature
Some reasons:

  • raw identifiers are supposed to be used for keyword-like identifiers that quite rare case. And in this case, folding of r# can be misleading because folded item name conflicts with the corresponding keyword
  • current implementation hides r# prefix completely. It can be confusing while modification of the identifier and navigation when the prefix appears suddenly.
    2019-04-29 18 16 11

But other implementations don't make sense at all because r# prefix is already very short

Due to the described reasons, we cannot turn on it by default (in this PR is just turned off) and looks like won't do it in the future. Including settings complication, nobody won't discover it.

Finally:

  • usefulness is rather ambiguous
  • it's not discoverable because it's turned off by default

So I'm going to revert these changes

@ice1000 Sorry for so non-instant feedback

bors bot added a commit that referenced this issue Apr 29, 2019
3773: TYPE: Revert raw identifiers folding r=mchernyavsky a=Undin

Reverts changes from #3759

See #3759 (comment) for more details

Co-authored-by: Arseniy Pendryak <a.pendryak@yandex.ru>
@ice1000
Copy link
Contributor Author

@ice1000 ice1000 commented Apr 29, 2019

Yeah that's what I heard from my friends too. No problem, that's reasonable.

@mchernyavsky mchernyavsky added this to the v98 milestone May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants