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

Add inline comments support to the xcconfig parser #604

Merged
merged 6 commits into from
Feb 18, 2021

Conversation

dive
Copy link
Contributor

@dive dive commented Feb 15, 2021

Resolves #602

Short description 📝

Add support for inline comments in xcconfig files.

Solution 📦

Extend the existed regular expression to support xcconfig comments and use them as a full-stop condition.

Implementation 👩‍💻👨‍💻

XCConfig.swift regular expression (settingRegex) extended with a positive lookahead condition as the last statement. Previously, the expression handles the end of the line ('$') as a final grouping statement and now it parses up until the end of the line or up to the comment indication (//) with the (?=$|\\/\\/) group.

  • Update XCConfigTests with failing scenarios
  • Adjust the settingRegex NSRegularExpression to support comments
  • Extend XCConfigTests with edge-cases

P.S. You can play with the updated regular expression here.

@codecov
Copy link

codecov bot commented Feb 15, 2021

Codecov Report

Merging #604 (10e11f3) into main (4b14bb1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #604   +/-   ##
=======================================
  Coverage   84.24%   84.25%           
=======================================
  Files         154      154           
  Lines        8672     8674    +2     
=======================================
+ Hits         7306     7308    +2     
  Misses       1366     1366           
Impacted Files Coverage Δ
Sources/XcodeProj/Utils/XCConfig.swift 92.47% <ø> (ø)
Tests/XcodeProjTests/Utils/XCConfigTests.swift 97.50% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b14bb1...10e11f3. Read the comment docs.

@adellibovi
Copy link
Collaborator

Nice fix @dive!

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Would you mind updating the changelog @dive ? We can merge afterward.

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Nice! This probably will not catch another edge case where the setting is followed by multiline comment /* but I am not sure if that's something we want to account for

@dive
Copy link
Contributor Author

dive commented Feb 18, 2021

Would you mind updating the changelog @dive ? We can merge afterward.

@pepibumur, added. Thanks!

Nice! This probably will not catch another edge case where the setting is followed by multiline comment /* but I am not sure if that's something we want to account for

@fortmarek, as far as I know, xcconfig does not support multiline comments. There is no official specification but Xcode ignores them for sure. The only additional edge-case I am aware of is NAME="https://example.com" case but even the current Xcode parser does not allow such entries in xcconfig.

@pepicrft pepicrft merged commit f0b1d1b into tuist:main Feb 18, 2021
@fortmarek
Copy link
Member

Thanks @dive for the explanation, makes sense 👍

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.

Remove end of line comments from xcconfig build settings
4 participants