BartyCrouch icon indicating copy to clipboard operation
BartyCrouch copied to clipboard

Remove empty lines from source files

Open JohNan opened this issue 2 years ago • 4 comments

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

JohNan avatar Jun 08 '22 09:06 JohNan

@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.

Jeehut avatar Jun 08 '22 22:06 Jeehut

@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.

JohNan avatar Jun 09 '22 08:06 JohNan

@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 avatar Jun 09 '22 12:06 Jeehut

@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.

Jeehut avatar Sep 25 '22 08:09 Jeehut