Finish pspdfkit labs search #703

tmcw · 2016-12-16T02:32:44Z

This branch addresses all of the current comments on #626: namely

Fix rubocop issues
Add changelog entry
Remove unused tempfile entry
Change --searchable to --unsearchable

The only thing that currently is tricky is that the self.document method is pushed from 20 to 21 lines, so it gets caught by rubocop. I see no clear way to make it shorter without making the code less direct, so happy to either adjust rubocop or take any approach the maintainers are happy with.

DangerCI · 2016-12-16T02:52:23Z

	1 Warning
⚠️	Big PR

Generated by 🚫 danger

tmcw · 2016-12-16T03:10:18Z

Noting that tests are still failing: I didn't go through the dance of updating CI test fixtures. Looks like the steps to do that are in CONTRIBUTING.md and will require committing changes to the text fixture repo. (also, noting that to get the rake task to work i had to gem install bacon manually)

Test failures at: https://travis-ci.org/realm/jazzy/builds/184429638

@captainbarbosa want to take the baton in this magical relay race of open source feature production?

captainbarbosa · 2016-12-16T20:48:15Z

@tmcw I've updated the integration specs per the instructions in CONTRIBUTING.md

@jpsim I think this is ready to go, can we get push access so we can complete the final steps?

Also to note - while we were updating the integration spec, we had to update MiscJazzyFeatures Xcode project to explicitly set the SWIFT_VERSION to legacy:

=== BUILD TARGET MiscJazzyFeatures OF PROJECT MiscJazzyFeatures WITH THE DEFAULT CONFIGURATION (Release) ===

Check dependencies
“Use Legacy Swift Language Version” (SWIFT_VERSION) is required to be configured correctly for targets which use Swift. Use the [Edit > Convert > To Current Swift Syntax…] menu to choose a Swift version or use the Build Settings editor to configure the build setting directly.

** BUILD FAILED **

I also had to recursively clone the submodules, which wasn't mentioned in the instructions within CONTRIBUTING.md - I can post a separate PR to include this.

jpsim · 2016-12-16T20:51:28Z

Oh hai mapbox! 👋

Thanks for pushing this forward! And of course thanks to @esad for the initial implementation.

I also had to recursively clone the submodules, which wasn't mentioned in the instructions within CONTRIBUTING.md - I can post a separate PR to include this.

Oh, that'd be great! Yes please.

I've just granted @captainbarbosa and @tmcw push access to both this repo and the integration specs repo.

jpsim

Just a few changes to make before this can go live. Nicely done @esad @tmcw @captainbarbosa !

jpsim · 2016-12-16T20:54:13Z

lib/jazzy/config.rb

@@ -246,6 +246,11 @@ def expand_path(path)
                   'https://github.com/realm/realm-cocoa/tree/v0.87.1)'

    # ──────── Doc generation options ────────
+    config_attr :unsearchable,


I think this should be named either disable_search/--disable-search or skip_search/--skip-search to match the other options.

jpsim · 2016-12-16T20:55:20Z

CHANGELOG.md

@@ -6,7 +6,8 @@

 ##### Enhancements

-* None.
+* By default, docs are now searchable. A new option, `--unsearchable`, lets you


Only the fullwidth theme appears to be searchable. Also, this should be reformatted to match the standard changelog entry format defined in https://github.com/realm/jazzy/blob/master/CONTRIBUTING.md#tracking-changes

With credit given to @esad, @tmcw and @captainbarbosa

jpsim · 2016-12-16T20:56:46Z

lib/jazzy/themes/fullwidth/assets/js/lunr.min.js

@@ -0,0 +1,7 @@
+/**
+ * lunr - http://lunrjs.com - A bit like Solr, but much smaller and not as bright - 0.7.0


Should we update this to 0.7.2? https://github.com/olivernn/lunr.js/releases/tag/v0.7.2

jpsim · 2016-12-22T00:20:09Z

Would you like me to address the remaining feedback points, or should I leave that to you @tmcw?

captainbarbosa · 2017-01-06T18:46:15Z

@jpsim I made the changes as advised from your review - let me know if there's anything else I can do!

jpsim · 2017-01-06T18:50:23Z

Thanks @captainbarbosa. There's now a merge conflict with the changelog and the integration specs need to be rebuilt again.

friedbunny · 2017-01-06T23:10:45Z

Here’s the rubocop failure @tmcw mentioned:

lib/jazzy/doc_builder.rb:322:5: C: Method has too many lines. [21/20]
    def self.document(source_module, doc_model, path_to_root) ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

25 files inspected, 1 offense detected
RuboCop failed!

friedbunny · 2017-01-07T00:03:44Z

@jpsim We’re having some issues regenerating the integration specs. The new files from this PR are successfully added, but the docs generated for Siesta are inexplicably having sections removed:

diff --git a/document_siesta/after/api-docs/Classes/Resource.html b/document_siesta/after/api-docs/Classes/Resource.html
index 1c574ce..0fd0aee 100644
--- a/document_siesta/after/api-docs/Classes/Resource.html
+++ b/document_siesta/after/api-docs/Classes/Resource.html
@@ -740,29 +740,6 @@ up to you to canonicalize your parameter order.</p>
 <p>Note that, unlike load() and loadIfNeeded(), this method does <em>not</em> update latestData or latestError,
 and does not notify resource observers about the result.</p>
 
-<div class="aside aside-parameter">
-    <p class="aside-title">Parameter</p>
-    Parameter method: The HTTP verb to use for the request
-
-</div>
-
-<div class="aside aside-parameter">
-    <p class="aside-title">Parameter</p>
-    <p>Parameter requestMutation:
-An optional callback to change details of the request before it is sent. For example:</p>
-<pre class="highlight plaintext"><code>request(.POST) { nsreq in
-  nsreq.HTTPBody = imageData
-  nsreq.addValue(
-    "image/png",
-    forHTTPHeaderField:
-      "Content-Type")
-}
-</code></pre>
-
-<p>Does nothing by default.</p>
-
-</div>
-

Full diff of the spec regeneration is available here.

Siesta does not use the full-width theme and I therefore wouldn’t expect any changes to be made. document_siesta/after/execution_output.txt has only one lined changed and that one is expected: “building search index”.

Here’s the output from bundle exec rake rebuild_integration_fixtures. The relevant Siesta lines don’t have much information:

    Creates Siesta docs
      ✓ $ jazzy --output api-docs
      - execution_output.txt [FAILED]
      ✓ should not produce unexpected files

friedbunny · 2017-01-07T00:17:51Z

The above omissions do not happen when using 0.7.3, but still happen on this branch even when the --disable-search flag is used. 😞

jpsim · 2017-01-07T00:37:27Z

Yeah, the missing Siesta declaration is definitely a regression in this branch. I'm looking into it now.

jpsim · 2017-01-07T00:59:28Z

I'm guessing at this point that SearchBuilder.build(...) somehow mutates sourcemodule in such a way to remove the asides used by Siesta, but I don't see how it could actually do that.

1ec5 · 2017-01-07T01:07:37Z

The Siesta deletions aren't new: they started happening shortly after the latest release, but before my recent PRs: #702 (comment)

Do the integration specs pull in the latest version of the Siesta package, which could've changed, or does this necessarily point to a regression in jazzy?

jpsim · 2017-01-07T01:11:36Z

I think these specific siesta deletions are different, as running with the following patch preserves those asides:

--- a/lib/jazzy/doc_builder.rb
+++ b/lib/jazzy/doc_builder.rb
@@ -125,11 +125,6 @@ module Jazzy
       output_dir = options.output
       build_docs(output_dir, source_module.docs, source_module)
 
-      unless options.disable_search
-        warn 'building search index'
-        SearchBuilder.build(source_module, output_dir)
-      end
-
       copy_assets(output_dir)
 
       DocsetBuilder.new(output_dir, source_module).build!

jpsim · 2017-01-07T01:22:21Z

Actually, that's not even it. If I just run the siesta specs with this branch, the asides are also preserved. They just disappear if I run the whole test suite. Leading me now to think that it's some other mutation that's causing this...

friedbunny · 2017-01-09T08:31:51Z

The vanishing Siesta asides bisected to #697 — the integration spec apparently wasn’t updated properly for that PR. I’ve proposed realm/jazzy-integration-specs#25 to fix the issue, which simply accepts the deletions as proper.

friedbunny · 2017-01-09T09:30:24Z

In 7825086, I went ahead and disabled the rubocop method length failure from #703 (comment).

Please let me know if there’s anything I’m missing, but I believe this is what’s left to do:

Merge Belated updates for realm/jazzy#697 jazzy-integration-specs#25.
Push rebuilt integration specs for this PR to realm/jazzy-integration-specs branch.
Update integration specs submodule commit hash in this PR.
Merge rebuilt integration specs.
🥃

Excited to get this train into the station! 🙇

SDGGiesbrecht · 2017-01-09T19:20:39Z

Thanks for fixing the specs.

jpsim · 2017-01-09T21:23:42Z

Your prognosis and fixes look right to me @friedbunny! Thanks for everything. Let's get this green by pushing the rebuilt integration specs and pointing this branch to it.

friedbunny · 2017-01-09T21:36:57Z

Awesome! I don’t have push access, but I was going to tag @captainbarbosa in again, anyway. 😁

jpsim · 2017-01-09T21:50:28Z

I gave you write access in case that helps moving forward.

Fixes from add finishing touches to PSPDFKit-search: * Fix rubocop issues * Add changelog entry * Remove unused tempfile entry * Change --searchable to --unsearchable

jpsim · 2017-01-10T06:13:26Z

You got so close! Will finish finishing this finishment in #725.

realm-ci assigned tmcw Dec 16, 2016

captainbarbosa mentioned this pull request Dec 16, 2016

Update CONTRIBUTING.md #705

Merged

jpsim requested changes Dec 16, 2016

View reviewed changes

esad and others added 5 commits January 9, 2017 17:54

Add --unsearchable flag and make docs searchable by default

e7f939f

Fixes from add finishing touches to PSPDFKit-search: * Fix rubocop issues * Add changelog entry * Remove unused tempfile entry * Change --searchable to --unsearchable

Rename unsearchable --> disable_search

912b98b

Update lunr.js from 0.7.0 to 0.7.2

a5fb8ee

Disable rubocop method length rule for self.document(...)

33f13b6

Update integration specs

cf94937

jpsim closed this Jan 10, 2017

1ec5 deleted the finish-pspdfkit-labs-search branch January 11, 2017 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish pspdfkit labs search #703

Finish pspdfkit labs search #703

tmcw commented Dec 16, 2016

DangerCI commented Dec 16, 2016 •

edited

Loading

tmcw commented Dec 16, 2016 •

edited

Loading

captainbarbosa commented Dec 16, 2016

jpsim commented Dec 16, 2016

jpsim left a comment

jpsim Dec 16, 2016

jpsim Dec 16, 2016

jpsim Dec 16, 2016

jpsim commented Dec 22, 2016 •

edited

Loading

captainbarbosa commented Jan 6, 2017

jpsim commented Jan 6, 2017

friedbunny commented Jan 6, 2017

friedbunny commented Jan 7, 2017

friedbunny commented Jan 7, 2017

jpsim commented Jan 7, 2017

jpsim commented Jan 7, 2017

1ec5 commented Jan 7, 2017

jpsim commented Jan 7, 2017

jpsim commented Jan 7, 2017

friedbunny commented Jan 9, 2017

friedbunny commented Jan 9, 2017 •

edited

Loading

SDGGiesbrecht commented Jan 9, 2017

jpsim commented Jan 9, 2017

friedbunny commented Jan 9, 2017

jpsim commented Jan 9, 2017

jpsim commented Jan 10, 2017

		@@ -0,0 +1,7 @@
		/**
		* lunr - http://lunrjs.com - A bit like Solr, but much smaller and not as bright - 0.7.0

Finish pspdfkit labs search #703

Finish pspdfkit labs search #703

Conversation

tmcw commented Dec 16, 2016

DangerCI commented Dec 16, 2016 • edited Loading

tmcw commented Dec 16, 2016 • edited Loading

captainbarbosa commented Dec 16, 2016

jpsim commented Dec 16, 2016

jpsim left a comment

Choose a reason for hiding this comment

jpsim Dec 16, 2016

Choose a reason for hiding this comment

jpsim Dec 16, 2016

Choose a reason for hiding this comment

jpsim Dec 16, 2016

Choose a reason for hiding this comment

jpsim commented Dec 22, 2016 • edited Loading

captainbarbosa commented Jan 6, 2017

jpsim commented Jan 6, 2017

friedbunny commented Jan 6, 2017

friedbunny commented Jan 7, 2017

friedbunny commented Jan 7, 2017

jpsim commented Jan 7, 2017

jpsim commented Jan 7, 2017

1ec5 commented Jan 7, 2017

jpsim commented Jan 7, 2017

jpsim commented Jan 7, 2017

friedbunny commented Jan 9, 2017

friedbunny commented Jan 9, 2017 • edited Loading

SDGGiesbrecht commented Jan 9, 2017

jpsim commented Jan 9, 2017

friedbunny commented Jan 9, 2017

jpsim commented Jan 9, 2017

jpsim commented Jan 10, 2017

DangerCI commented Dec 16, 2016 •

edited

Loading

tmcw commented Dec 16, 2016 •

edited

Loading

jpsim commented Dec 22, 2016 •

edited

Loading

friedbunny commented Jan 9, 2017 •

edited

Loading