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

feat: Allow to enforce ellipsis instead of three dots #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

susnux
Copy link

@susnux susnux commented Aug 30, 2024

Our translation and design guidelines say to use ellipsis instead of three dots, yet some code is using three dots instead of the ellipsis character. This adds a custom fixer and rule to allow auto-fixing those strings.

I did not default enable it, because it is a risky rule that might change behavior (e.g. string comparison).
Feel free to dismiss if you feel like this should not be enforced.

@susnux susnux added the enhancement New feature or request label Aug 30, 2024
Our translation and design guidelines say to use ellipsis instead of three dots,
yet some code is using three dots instead of the ellipsis character.
This adds a custom fixer and rule to allow auto-fixing those strings.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the feat/allow-enforce-ellipsis branch from 7b39377 to 0ceffd5 Compare August 30, 2024 15:31
@kesselb
Copy link
Contributor

kesselb commented Aug 30, 2024

Good idea, especially because it's also enforced for apps then.

Do we still run https://github.com/nextcloud/server/blob/master/build/triple-dot-checker.php?

@kesselb
Copy link
Contributor

kesselb commented Aug 30, 2024

~30 files in server.

image

That's a bit weird, but okay.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

I'm on the fence with this.

As Daniel shows, this might influence strings that are not user visible, and this is more of a semantic check/fix. I think the cs fixer should only fix syntax formatting.

Should this be a Psalm rule like build/psalm/OcpSinceChecker.php?

@nickvergessen
Copy link
Member

Or (when it's only about user facing strings) in the translation checker?
https://github.com/nextcloud/server/blob/master/build/translation-checker.php#L65

@susnux
Copy link
Author

susnux commented Sep 9, 2024

Or (when it's only about user facing strings) in the translation checker?

The idea was to also check for apps.

Should this be a Psalm rule like build/psalm/OcpSinceChecker.php?

If that makes more sense for you, then sure. I can look into that when I have some more time.

@nickvergessen
Copy link
Member

The idea was to also check for apps.

Yeah the above and some future checks should be done for all apps as well

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

Successfully merging this pull request may close these issues.

5 participants