Changes search index format to fasten `pod search --full` command. #265

manuyavuz · 2015-09-23T19:10:46Z

Detailed discussion can be found here: CocoaPods/cocoapods-search#8

Previous implementation was having the drawback of traversing and performing gsub on all index strings for each pod specification.
New implementation stores words as keys and list of pods containing corresponding word inside their specification as the values for corresponding hash keys. By this way, sources manager can perform a faster search operation.

segiddins · 2015-09-24T15:55:08Z

lib/cocoapods-core/source/aggregate.rb

      rescue
        CoreUI.warn "Skipping `#{set.name}` because the podspec contains " \
          'errors.'
-        result
+        return []


return keyword is superfluous

segiddins · 2015-10-08T15:28:48Z

spec/source/aggregate_spec.rb

+        index['BananaLib'].should == ['BananaLib']
+        index['JSONKit'].should == ['JSONKit']
+        index['JSONSpec'].should == ['JSONSpec']
+        index['Faulty_spec'].should.nil?


should.be.nil

manuyavuz · 2015-10-16T15:22:04Z

Got an offense from RuboCop about using return while returning two parameters.

I think I have to use return in this case, so we can ignore it.

Or maybe I can return an array value including two parameters. Which makes more sense?

segiddins · 2015-10-16T22:22:58Z

lib/cocoapods-core/source.rb

+        output = `git pull --ff-only`
+        changed_spec_paths = (`git diff --name-only #{prev_commit_hash}..HEAD`).strip.split("\n")
+      end
+      return output, changed_spec_paths


[output, changed_spec_paths], no return

segiddins · 2015-10-19T21:49:24Z

@manuyavuz are these PRs ready for final review?

manuyavuz · 2015-10-19T21:53:40Z

It lacks specs but I think we should be in the same line about the implementation before starting specs. So I think it's ready for review regarng implementation. What do you think?

segiddins · 2015-10-19T21:54:51Z

lib/cocoapods-core/source.rb

+    #          Returns the list of changed spec paths.
+    #
+    def update(show_output = false)
+      changed_spec_paths = nil


this should be []

manuyavuz · 2015-10-20T07:34:24Z

If we're OK with the current state, I'll move on adding specs. Just let me know.

segiddins · 2015-10-20T13:51:30Z

I am, but maybe @floere has some thoughts

floere · 2015-10-20T14:25:30Z

lib/cocoapods-core/source/aggregate.rb

-        result
+        string = set.name.dup
+        if spec.summary
+          string << ' ' << spec.summary


In Ruby, it's generally better to avoid string << ' ' << spec.summary because with each <<, a String is created. Use arrays to << the strings, then join once at the end.

Or better yet, in this case, only split where necessary (e.g. in summary) and append the result to the array using +=, then uniq at the end.

floere · 2015-10-20T14:26:24Z

Only one comment, repeated here, refering to the latest commit where this code is located, string << ' ' << spec.summary:
In Ruby, it's generally better to avoid string << ' ' << spec.summary because with each <<, a String is created. Use arrays to << the strings, then join once at the end.
Or better yet, in this case, only split where necessary (e.g. in summary) and append the result to the array using +=, then uniq at the end.

floere · 2015-10-20T14:27:02Z

Happy to wait for the specs now 😊 Thanks for all your work!!!

manuyavuz · 2015-10-20T14:46:26Z

Great hint @floere 👏🏼

I'm starting to specs 💥

manuyavuz · 2015-10-21T18:09:14Z

Done with specs. You can check out and review now.

I'll rewrite history a little to have a convenient history. Other than that, I think this is ready to go to master.

segiddins · 2015-10-22T02:45:25Z

Looks amazing to me 👍
But I also want to let @floere have one last look

segiddins · 2015-10-22T02:49:03Z

lib/cocoapods-core/source.rb

+      (`git rev-parse HEAD` || '').strip
+    end
+
+    def update_git_repo


we should override this in CocoaPods/CocoaPods to use executable, so the output is still streamed?

I don't think this is needed because it's output is handled and streamed in update method above.
Look at the lines between 292-300. It uses CoreUI which is handled by CocoaPods by default in user_interface.rb as far as I understand.

No, the output here will no longer be streamed -- it will only be printed after the command is finished.

You're right, this prints after command is finished. So what should we do?

I think we're gonna remove CoreUI.puts output if show_output part in update method here in order not to have duplicate output. However, I didn't fully get how to override a method of Source class in cocoapods code (probably in sources_manager.rb but how)?

I'm not a native ruby person so probably I miss sth :)

manuyavuz · 2015-10-22T19:54:57Z

I think I've found the way to override update_git_repo method in CocoaPods/CocoaPods in order to enable output streaming. Sample code:

  class Source
    extend Executable
    executable :git

    alias_method :_original_update_git_repo, :update_git_repo

    def update_git_repo
      git %w(pull --ff-only)
    end
  end

I will add to CocoaPods/CocoaPods#4249 in next commit.

segiddins · 2015-10-22T20:04:19Z

@manuyavuz no need to alias

manuyavuz · 2015-10-22T20:20:08Z

It was really simple then 👍
We also need to move UI.warn logic into here because Executable does not set $? so CoreUI.warn in here does not called when pull fails.

Regarding this, we may completely remove UI printing here, and handle it in sources_manager again, this time in above overridden method. What do you think?

segiddins · 2015-10-22T20:55:06Z

Sure

floere · 2015-10-22T21:18:46Z

lib/cocoapods-core/source/aggregate.rb

+        result = {}
+        sets.each do |set|
+          word_list_from_set(set).each do |w|
+            (result[w] ||= []).push(set.name)


Depending on the average number of words in a wordlist, using the set name/identifier as a symbol will use a bit less space. If it's only used to generate the file index, and not for storing it in memory (and search operations) it's not that crucial.

floere · 2015-10-22T21:24:55Z

Looks good – only had comments on the index creation. Thanks again for your work, @manuyavuz! 🚀

manuyavuz · 2015-10-23T09:10:12Z

I believe we need a spec for this method, since the index data is so crucial for later searching.

@floere Actually the new specs I've added implicitly verify this method's correctness. What type of checks you think needed more? Like specs having no description, or authors part? Or like checking if strings are splitted correctly?

Could you share if you have edge cases in your mind which you feel does not being tested now? I can add them accordingly.

floere · 2015-10-25T12:08:11Z

@manuyavuz Just checking how strings are split. Very simple, maybe something as simple as a.b-c/d -> %w(a b c d). This will help us later find bugs (since people like their pods be found and will complain if they can't), and guard against unintended consequences to tokenization changes. Something which the implicit tests can't do very well.

- Previous implementation was having the drawback of traversing and performing `gsub` on all index strings for each pod specification. - New implementation stores words as keys and list of pods containing corresponding word inside their specification as the values for corresponding hash keys. By this way, sources manager can perform a faster search operation.

manuyavuz · 2015-10-26T17:15:04Z

@floere I've added a spec for word_list_from_set method. Please tell if anything feels wrong or redundant.

floere · 2015-10-26T17:21:53Z

@manuyavuz All good! 👍 🚀

manuyavuz · 2015-10-26T17:26:35Z

Assuming this is ready to ⛵️, should I wait this to be merged in order to finish the last commit in CocoaPods/CocoaPods#4249? You remember I was directed Core repo to my branch for specs to be passed. If we merge this, I can revert it and submit last commit, so that we see green lights from boss travis.

manuyavuz · 2015-10-27T10:49:11Z

Added changelog.

segiddins · 2015-10-27T14:31:49Z

🚀 🚢

Changes search index format to fasten `pod search --full` command.

…formance Changes search index format to fasten `pod search --full` command.

segiddins reviewed Sep 24, 2015
View reviewed changes

This was referenced Oct 4, 2015

Changes search index format to fasten pod search --full command. CocoaPods/CocoaPods#4249

Merged

Improving search performance while using --full flag CocoaPods/cocoapods-search#8

Closed

segiddins reviewed Oct 8, 2015
View reviewed changes

segiddins reviewed Oct 16, 2015
View reviewed changes

segiddins reviewed Oct 19, 2015
View reviewed changes

floere reviewed Oct 20, 2015
View reviewed changes

segiddins reviewed Oct 22, 2015
View reviewed changes

floere reviewed Oct 22, 2015
View reviewed changes

manuyavuz mentioned this pull request Oct 23, 2015

Improve tokenization policy CocoaPods/cocoapods-search#9

Open

manuyavuz added 9 commits October 26, 2015 20:12

Move source update logic to source.rb

0e2a586

Return nil from representative_set if no pod found with given name.

e08c610

Forward stderr for git pull command.

e7cf3c4

Move UI logic of update to Core.

4bf1127

Add specs for source update

4403bf3

Simplify UI logic in source update.

192defe

Use symbols to reference pods in search index

a4f87c9

Add spec for vocabulary creation

f75d9ce

Add CHANGELOG

3ba0762

segiddins added a commit that referenced this pull request Oct 27, 2015

Merge pull request #265 from manuyavuz/improvement/searchPerformance

d66fcb7

Changes search index format to fasten `pod search --full` command.

segiddins merged commit d66fcb7 into CocoaPods:master Oct 27, 2015

Ashton-W pushed a commit to Ashton-W/Core that referenced this pull request Nov 2, 2015

Merge pull request CocoaPods#265 from manuyavuz/improvement/searchPer…

d1af69f

…formance Changes search index format to fasten `pod search --full` command.

manuyavuz deleted the improvement/searchPerformance branch November 3, 2015 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes search index format to fasten `pod search --full` command. #265

Changes search index format to fasten `pod search --full` command. #265

manuyavuz commented Sep 23, 2015

segiddins Sep 24, 2015

segiddins Oct 8, 2015

manuyavuz commented Oct 16, 2015

segiddins Oct 16, 2015

segiddins commented Oct 19, 2015

manuyavuz commented Oct 19, 2015

segiddins Oct 19, 2015

manuyavuz commented Oct 20, 2015

segiddins commented Oct 20, 2015

floere Oct 20, 2015

floere commented Oct 20, 2015

floere commented Oct 20, 2015

manuyavuz commented Oct 20, 2015

manuyavuz commented Oct 21, 2015

segiddins commented Oct 22, 2015

segiddins Oct 22, 2015

manuyavuz Oct 22, 2015

segiddins Oct 22, 2015

manuyavuz Oct 22, 2015

manuyavuz commented Oct 22, 2015

segiddins commented Oct 22, 2015

manuyavuz commented Oct 22, 2015

segiddins commented Oct 22, 2015

floere Oct 22, 2015

floere commented Oct 22, 2015

manuyavuz commented Oct 23, 2015

floere commented Oct 25, 2015

manuyavuz commented Oct 26, 2015

floere commented Oct 26, 2015

manuyavuz commented Oct 26, 2015

manuyavuz commented Oct 27, 2015

segiddins commented Oct 27, 2015

Changes search index format to fasten pod search --full command. #265

Changes search index format to fasten pod search --full command. #265

Conversation

manuyavuz commented Sep 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuyavuz commented Oct 16, 2015

Choose a reason for hiding this comment

segiddins commented Oct 19, 2015

manuyavuz commented Oct 19, 2015

Choose a reason for hiding this comment

manuyavuz commented Oct 20, 2015

segiddins commented Oct 20, 2015

Choose a reason for hiding this comment

floere commented Oct 20, 2015

floere commented Oct 20, 2015

manuyavuz commented Oct 20, 2015

manuyavuz commented Oct 21, 2015

segiddins commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuyavuz commented Oct 22, 2015

segiddins commented Oct 22, 2015

manuyavuz commented Oct 22, 2015

segiddins commented Oct 22, 2015

Choose a reason for hiding this comment

floere commented Oct 22, 2015

manuyavuz commented Oct 23, 2015

floere commented Oct 25, 2015

manuyavuz commented Oct 26, 2015

floere commented Oct 26, 2015

manuyavuz commented Oct 26, 2015

manuyavuz commented Oct 27, 2015

segiddins commented Oct 27, 2015

Changes search index format to fasten `pod search --full` command. #265

Changes search index format to fasten `pod search --full` command. #265