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

asciidoc: add cleanspaces option to remove extra spaces #481

Merged
merged 1 commit into from
May 9, 2024

Conversation

jnavila
Copy link
Collaborator

@jnavila jnavila commented Feb 28, 2024

Extra spaces are removed in no-wrap segments, so that the output conforms to English typography rules.

This fixes GitHub #464

Extra spaces are removed in no-wrap segments, so that the output
conforms to English typography rules.

This fixes GitHub #464
@mquinson
Copy link
Owner

Hello Jean-Noël, thanks for this patch.

maybe the cleanup should be the default behavior for new users, don't you think?

I'm not completely decided, but it seems somehow better.

Mt

@jnavila
Copy link
Collaborator Author

jnavila commented Feb 29, 2024

I don't grasp the idea of "the default behavior for new users". Do you mean that we should somehow differentiate between creating the pot file and updating it? How can we do it, and save the actual state of the option?

For sure, for existing translation project, it is critical to retain the behavior of unset cleanspaces, unless the project manager actively enables it.

@mquinson
Copy link
Owner

mquinson commented Mar 4, 2024

I mean that I would like this behavior to be activated by default, with a big warning in the changelog saying to existing project that they may want to use that option to disable the behavior if they wish so.

I know it may be troublesome to some users, but I think that an optional cleanup is not very different from no cleanup at all.

Does it sound too harsh to you?

@jnavila
Copy link
Collaborator Author

jnavila commented Mar 4, 2024

OK, so your option is the harsh one 😆 ... Being the main developer of this feature and being plagued with this issue, I know that this can be cumbersome to change a lot of segments, only for some whitespace issues.

I don't know how you intend to warn user about this compatibility breakage. I would at least issue a warning when such issue is detected and extra whitespaces are detected and removed. So maybe, this option should be activated by default, generating a warning. When the option is set to true or false, the warning would disappear.

Personally, I understand your point: a sane behavior for new projects, but I don't like at all breaking the increasing number of projects already relying on po4a. In any case, it would be a breaking change that needs to be strongly advertised.

@jnavila
Copy link
Collaborator Author

jnavila commented Mar 4, 2024

Please note that right now, the patch cleans up the original string of its extra spaces but does not post process the translated string.

@mquinson mquinson merged commit 645a033 into master May 9, 2024
1 check passed
@mquinson
Copy link
Owner

mquinson commented May 9, 2024

Thanks a lot @jnavila, I merged this as is. You are right about not upsetting the translators by default.

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.

2 participants