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

Merge extension with extended type #343

Merged
merged 11 commits into from
Nov 10, 2015

Conversation

pcantrell
Copy link
Collaborator

Continuing #289 based off of master instead of swift-2.0.

@pcantrell
Copy link
Collaborator Author

I went ahead and got this one ready, now that @jpsim big Obj-C push is done. It’s now based off of master instead of swift-2.0, and the specs are up to date.

This is ready to merge if you’re happy with the output. As a reminder of issues from the older discussion:

end

def protocol?
kind =~ /^source\.lang\.swift\.decl\.protocol$/
Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems nicer/cleaner to just do a string comparison if we're matching for an exact string anyway?

@jpsim
Copy link
Collaborator

jpsim commented Nov 10, 2015

This is so so great. I'm still impressed at how concise you managed to keep this code.

👍 other than my very minor comments.

Also, although this is something I haven't done yet, there are lots of areas now where Swift processing differs from Objective-C processing, and we should at some point go through the codepaths to branch off where needed to keep things efficient.

@pcantrell
Copy link
Collaborator Author

This is so so great. I'm still impressed at how concise you managed to keep this code.

That’s kind of you. I actually found it a little hard to read on re-review, so I did a very light refactoring that I think helps.

@jpsim, I’ve made your very sensible fixes, so if CI likes it, we’re good to go.

@jpsim
Copy link
Collaborator

jpsim commented Nov 10, 2015

Looks like there are rubocop violations to address:

Offenses:
lib/jazzy/sourcekitten.rb:313:9: C: Align the operands of an expression in an assignment spanning multiple lines.
        .group_by { |d| deduplication_key(d) }
        ^^^^^^^^^
lib/jazzy/sourcekitten.rb:314:9: C: Align the operands of an expression in an assignment spanning multiple lines.
        .values
        ^^^^^^^
22 files inspected, 2 offenses detected
RuboCop failed!
The command "bundle exec rake spec" exited with 1.

@pcantrell
Copy link
Collaborator Author

Yeah, weird — why didn’t those show up locally? Fixing…

@pcantrell
Copy link
Collaborator Author

Not getting that rubocop failure locally. I notice that I have rubocop 0.26.1 locally, which is what’s in the Gemfile.lock, but Travis is downloading 0.34.2. ?!?

Just pushed a commit that’s a shot in the dark at what I think rubocop is asking for. We’ll see if it passes.

@jpsim
Copy link
Collaborator

jpsim commented Nov 10, 2015

Rubocop is pinned at 0.34.2 in the Gemfile.lock on master: https://github.com/realm/jazzy/blob/master/Gemfile.lock#L88

In any case, your shot in the dark passed! 🎉

I'll add a changelog entry for you in the spirit of getting this in.

jpsim added a commit that referenced this pull request Nov 10, 2015
@jpsim jpsim merged commit 9b719ca into realm:master Nov 10, 2015
@pcantrell
Copy link
Collaborator Author

GTG

@pcantrell
Copy link
Collaborator Author

Oh, right , changelog. Thanks.

jpsim added a commit that referenced this pull request Nov 10, 2015
@pcantrell pcantrell deleted the merge-extension-with-extended-type branch November 13, 2015 19:02
@pigeon-archive pigeon-archive modified the milestone: The Past Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants