Skip to content
This repository has been archived by the owner on Nov 3, 2020. It is now read-only.

Discussion: Pod version completion #313

Merged
merged 15 commits into from
May 23, 2016

Conversation

seanlabastille
Copy link
Contributor

Relating to #311, this is a rough implementation of looking up pod versions using the reflection service.

While I have some concerns relating to the editor changes —

  • some of the logic here could be factored out a little more, and
  • I'm not sure if this is the optimal way to call the reflection service and await a return value —

my primary concern is the changes needed in Fragaria to adjust when completions are allowed and if all options are returned with an empty prefix.

def self.pod_versions(podName)
sources_manager.aggregate.search_by_name(podName).first.versions.map do |version|
version.to_s
end
Copy link
Member

Choose a reason for hiding this comment

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

sources_manager.aggregate.search_by_name(podName).first.versions.map(&:to_s) is a tad more elegant 👍

@seanlabastille
Copy link
Contributor Author

I've reworked the fetching of the pod versions based on the current cursor position as suggested, this looks a bit nicer since the need to block on fetching the pod versions is also eliminated.


The only other thing I could think of here is that we change Fragaria's autocompletion to be async?

I'm not sure much can be done here as this is built on top of NSTextView's autocompletion.


However, I still think the tweak to SMLTextView's rangeForUserCompletion remains necessary.

@seanlabastille
Copy link
Contributor Author

The Travis builds seem to be erroring with a missing API token for danger. Is there something I should do about that?

@orta
Copy link
Member

orta commented May 10, 2016

That's not a worry - we don't expose Danger's auth to PRs ATM, which we should do I think

@orta
Copy link
Member

orta commented May 11, 2016

Sorry for the delay on this one, wanted to make sure a stable 1.0 is out, then can starting thinking about the next release

@@ -1267,6 +1267,15 @@ - (NSRange)rangeForUserCompletion
{
return NSMakeRange(NSNotFound, 0);
}

Copy link
Member

Choose a reason for hiding this comment

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

so this is the bit that worries me, the next time we do a pod install this gets overwritten.

In #333 there are some (now merged into master) functions added that may be able to replicate some of this work without having to make changes at SMLTextView level, any chance you can take a quick look? I'd like to get this merged so we can start looking at it more holistically for #311 and building an AST. It's easier to work with these things in place first IMO 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a look at those changes and how I can build on top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to make some progress here, however, this has run into some trouble.
There seem to be two options:

  • swizzle the implementation of the methods that I've adjusted
  • subclass SMLTextView with the changes

I thought subclassing would be preferable to swizzling. In the end I've discovered that I can't easily drop in a subclass for SMLTextView and that swizzling is already used (SMLTextView+CocoaPods.m).

I'll have another look at swizzling later.

@seanlabastille
Copy link
Contributor Author

This should be ready for another look.
I've moved the tweaks together with the existing swizzles on SMLTextView.

@CocoaPodsBarista
Copy link

CocoaPodsBarista commented May 22, 2016

      <tr>
    <td>:white_check_mark:</td>
    <td data-sticky="true"><del><p>Please include a CHANGELOG entry to credit yourself! 

You can find it at CHANGELOG.md.


        :white_check_mark: Jolly good show.
    
  </th>
 </tr>

Generated by 🚫 danger

@orta
Copy link
Member

orta commented May 22, 2016

OK, this looks great - I think Danger will pop up and warn you about a CHANGELOG entry

@orta
Copy link
Member

orta commented May 22, 2016

oh, yeah, there it is, so - just add that, then we're good to go - thanks!

@orta
Copy link
Member

orta commented May 23, 2016

Ace!

@orta orta merged commit 48be8a2 into CocoaPods:master May 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants