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

Remove empty lines from source files #255

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

Conversation

JohNan
Copy link

@JohNan JohNan commented Jun 8, 2022

Proposed Changes

When using the following options the empty lines was not removed in the source files

[update.normalize]
paths = ["./Sources"]
sourceLocale = "sv"
separateWithEmptyLine = false
sortByKeys = false
harmonizeWithSource = true

This PR make sure to rewrite the source files without empty lines in all cases when separateWithEmptyLine is disabled

@Jeehut
Copy link
Member

Jeehut commented Jun 8, 2022

@JohNan Thank you for bringing this issue up and providing a PR to fix.

I don't have much time this week due to WWDC, but wouldn't your implementation here rewrite the files each time, even when no changes occur when this new option is turned off? I just had a cursory look (and it's late, too), so I might have misunderstood things. But it looks to me like you're sacrificing a shortcut which can prevent rewriting files unnecessarily just for those who are migrating from the old option to the new.

Or let me ask differently: If a user would simply search & replace \n\n with \n in their project within all .strings files, wouldn't that resolve the issue better? I'd rather not sacrifice performance if this is only something relevant for people migrating, I'd rather provide a migration guide or a different solution that maybe analyzes the current files whitespacing situation whenever the setting is changed.

Just my initial thoughts.

@JohNan
Copy link
Author

JohNan commented Jun 9, 2022

@Jeehut You are right. It will rewrite all source files when this option is disabled, even if no changes has occurred. Is there a way of checking this?

This can probably be solved by a manual find and replace as you suggested, but in that case the docs should state that it only works with other options enabled to not make it confusing.

@Jeehut
Copy link
Member

Jeehut commented Jun 9, 2022

@JohNan One way to check if changes are needed would be if you searched each file for containing \n\n and if not, you'd not have to execute the rewrite. But you can also just post a new PR with the migration notice, wherever you think it makes sense and is most understandable. I could also copy a text you provide to the release where the feature was introduced.

@Jeehut
Copy link
Member

Jeehut commented Sep 25, 2022

@JohNan Please note that I added this requested feature to ReMafoX, the successor to BartyCrouch that I'm actively working on right now. This feature was added in the latest Beta 4 which you can try out now. ReMafoX has an improved overall workflow and comes with a lot of improvements over BartyCrouch and I will actively add features over time. See the Beta 1 post for a full set of features and a comparison with BartyCrouch.

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