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

Improve Swift attribute formatting #806

Merged
merged 1 commit into from
Sep 10, 2019
Merged

Conversation

johnfairh
Copy link
Contributor

Swift @available attributes can go wrong:
screen shot 2017-10-13 at 09 10 50
The rparen inside the quoted string is accidentally ending the @available( and messing up what follows.

I also feel that treating the entire thing as a single token looks poor because of the internal structure. So this PR just allows 'normal' lexing of the part in parens giving e.g.:
screen shot 2017-10-13 at 09 16 03
and
screen shot 2017-10-13 at 09 16 57
The majority of attributes do not have args and are unaffected.

(This is similar in approach to Apple's default format:
screen shot 2017-10-13 at 09 18 46)

This change also deals with whitespace + comments inside the attr args / between attr and paren.

If there is a reason why the entire thing should be lexed as a single token then I'd be happy to change this PR to fix just the rparen-matching issue!

// cc @radex who touched this last for any thoughts.

@radex
Copy link
Contributor

radex commented Oct 16, 2017

Makes sense to me!

I remember changing it to the way it is now for some reason… Maybe not a good reason — I don't remember. I'd just suggest taking a look at blame/file history to see if there isn't something worth considering.

@johnfairh
Copy link
Contributor Author

OK - thanks! The previous changes were about not hard-coding the attribute names, nothing in conflict with this one. So will leave the PR as-is.

@johnfairh
Copy link
Contributor Author

Rebased!

@lordcodes
Copy link
Contributor

@johnfairh @pyrmont Is this PR still good to go? Looks like a good change for the lexer.

@johnfairh
Copy link
Contributor Author

I live in hope :) I know they're working through them.

@lordcodes
Copy link
Contributor

Great, was checking the PR was up-to-date and ready to go!

@pyrmont
Copy link
Contributor

pyrmont commented Sep 10, 2019

@johnfairh Apologies for force pushing to your fork—there have been a few more changes since your last rebase and I wasn't able to get the test suite to pass anymore.

This all looks good to me. I'm just waiting for Travis to finish and then will merge in :)

@pyrmont pyrmont merged commit 9890431 into rouge-ruby:master Sep 10, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Sep 10, 2019

@johnfairh Sorry this took so long and thank you for being so patient. It's great to have Rouge now highlight this in a more readable way :)

@johnfairh johnfairh deleted the swift-attrs branch September 10, 2019 08:26
@johnfairh
Copy link
Contributor Author

No worries, thanks for reviving the project.

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.

4 participants