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

[FEATURE] Properties File Escaping #293

Merged
merged 27 commits into from
Jul 29, 2019
Merged

Conversation

tobiasso85
Copy link
Contributor

@tobiasso85 tobiasso85 commented Jul 16, 2019

Add processor which escapes non ASCII characters in resources.

As per RFC 0007: SAP/ui5-tooling#168

Add processor which escapes non ASCII characters in resources.
@coveralls
Copy link

coveralls commented Jul 16, 2019

Coverage Status

Coverage increased (+0.3%) to 86.253% when pulling 46a7742 on properties-file-escaping into df7c789 on master.

Add task escapeNonAsciiCharacters which uses stringEscaper to escape
non ascii characters. Made it generic using a pattern to re-use it for
potential other files.
Add task "escapeNonAsciiCharacters" to ApplicationBuilder and
LibraryBuilder.
Add non-ascii characters to i18n test files.
Change name of constant and adjust its jsdoc for clarification.
matz3
matz3 previously requested changes Jul 19, 2019
lib/processors/stringEscaper.js Outdated Show resolved Hide resolved
lib/processors/stringEscaper.js Outdated Show resolved Hide resolved
lib/tasks/escapeNonAsciiCharacters.js Outdated Show resolved Hide resolved
- Always escape chars
- Read files in source-encoding
@CLAassistant
Copy link

CLAassistant commented Jul 19, 2019

CLA assistant check
All committers have signed the CLA.

@Thodd
Copy link

Thodd commented Jul 19, 2019

@tobiasso85:
Sorry, but @matz3 and I broke some tests.
#notsorry

Fix expected outcome in test
@tobiasso85
Copy link
Contributor Author

@tobiasso85:
Sorry, but @matz3 and I broke some tests.
#notsorry

#fixedit #unbreakable

Adjust encoding parameter
Expose stringEscaper processor and escapeNonAsciiCharacters task via
index.js to be able to consume it from server module.
Add default to formatter.
Adjust tests.
lib/lbt/bundle/AutoSplitter.js Outdated Show resolved Hide resolved
lib/processors/stringEscaper.js Outdated Show resolved Hide resolved
Rename stringEscaper to nonAsciiEscaper since it only escapes non ascii
characters. Made encoding a required parameter for the task.
Added static method to nonAsciiEscaper processor since node.js encoding
does not support "ISO-8859-1" only knows it as "latin1" though.
Thodd
Thodd previously requested changes Jul 23, 2019
lib/processors/nonAsciiEscaper.js Outdated Show resolved Hide resolved
@tobiasso85 tobiasso85 dismissed matz3’s stale review July 23, 2019 11:55

please re-review

Set default in formatters for encoding value.
Add validation to formatters for encoding value.
Only apply encoding in AutoSplitter and Builder if encoding value is
specified.
ui5.yml.

It makes more sense to group it under configuration.
Include prefix and option name in escapeNonAsciiCharacters task
Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

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

I have not finished my review yet but these are the findings so far

test/lib/builder/builder.js Outdated Show resolved Hide resolved
test/lib/lbt/bundle/AutoSplitter.js Outdated Show resolved Hide resolved
test/lib/processors/nonAsciiEscaper.js Outdated Show resolved Hide resolved
test/lib/processors/nonAsciiEscaper.js Show resolved Hide resolved
test/lib/processors/nonAsciiEscaper.js Outdated Show resolved Hide resolved
lib/lbt/bundle/AutoSplitter.js Outdated Show resolved Hide resolved
lib/lbt/bundle/Builder.js Outdated Show resolved Hide resolved
test/lib/lbt/bundle/AutoSplitter.js Outdated Show resolved Hide resolved
test/lib/lbt/bundle/AutoSplitter.js Outdated Show resolved Hide resolved
test/lib/processors/nonAsciiEscaper.js Outdated Show resolved Hide resolved
matz3
matz3 previously requested changes Jul 26, 2019
lib/lbt/utils/escapePropertiesFiles.js Outdated Show resolved Hide resolved
lib/lbt/bundle/AutoSplitter.js Outdated Show resolved Hide resolved
propertiesFileEncoding -> propertiesSourceFileEncoding
propertiesSourceFileEncoding -> propertiesFileSourceEncoding
@tobiasso85 tobiasso85 requested a review from matz3 July 26, 2019 13:55
test/lib/builder/builder.js Outdated Show resolved Hide resolved
Separated default encoding tests
@RandomByte
Copy link
Member

Fine for me. Please use "Squash and merge"!

@tobiasso85 tobiasso85 merged commit 9d213ce into master Jul 29, 2019
@tobiasso85 tobiasso85 deleted the properties-file-escaping branch July 29, 2019 14:39
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.

6 participants