Refactor tags #269

dometto · 2017-04-23T21:57:46Z

This refactor shaves about 5 seconds of load time for a page with a ridiculous amount (1000) of tags (~25 seconds vs. ~30), and (hopefully) improves readability and maintainability. To further improve performance I would like to eventually collapse the search for files and pages so we don't traverse the repository twice, and of course implement caching of the repo tree map. But that requires refactoring the Page and File classes, and implementing the idea for a Gollum::IO class...

Some breaking changes:

syntax for link attributes changed from image.jpg|frame|alt=Bla to image.jpg|frame, alt=Bla
image links and file links and are now also reversed if @markup.reverse_links? (previously only for page links and external links). This is because reversal is now handled at the start of the link processing rather than in each method separately.
a page link to waa.md would first be interpreted as a relative link, and if that didn't find a page, it would be interpreted as an absolute link (to /waa.md). So in cases of broken links the repository was traversed twice. I've disabled this, so an absolute link now requires an explicit /. However, this change doesn't break any tests.

…f link

dometto · 2017-04-23T21:59:01Z

Todo: update inline docs for tag filter.

bartkamphorst · 2017-04-24T06:39:28Z

lib/gollum-lib/filter/tags.rb

@@ -4,7 +4,8 @@
 class Gollum::Filter::Tags < Gollum::Filter
  # Extract all tags into the tagmap and replace with placeholders.
  def extract(data)
-    return data if @markup.format == :txt || @markup.format == :asciidoc
+    return data if @markup.skip_tags?


Expresses the intent much clearer, nice. 👍

bartkamphorst · 2017-04-24T09:28:47Z

lib/gollum-lib/filter/tags.rb

+  end
+
+  def process_link_tag(link_part, extra)
+    process_file_link_tag(link_part, extra) || process_page_link_tag(link_part, extra)


This means that all tags are processed as file links initially. Is there any way to avoid this?

Yes, that's what bothered me originally. I would like to just walk through the repo searching for the requested paths, returning Page objects if the found object has a valid page extensions and File objects otherwise. Then process_link_tag should choose what to with each link based on what kind of object it gets back (if any).

But I think that may require a new approach to Page and File objects altogether. Perhaps have BlobEntry superclass of which Page and File are both subclasses, with a common find-and-construct-from-path method. Or just have the BlobEntry class with a #file? and #page? method?

bartkamphorst · 2017-04-24T09:29:41Z

lib/gollum-lib/filter/tags.rb

@@ -98,10 +109,9 @@ def process_tag(tag)
  #   if it is not.
  #
  def process_include_tag(tag)
-    return unless /^include:/.match(tag)
+    return html_error('Cannot process include directive: no page name given') if tag.length <= 8


What is this magic number 8?

:) Point taken 👍

bartkamphorst · 2017-04-24T09:31:07Z

lib/gollum-lib/filter/tags.rb

    page_name          = tag[8..-1]
-    resolved_page_name = ::File.expand_path(page_name, "/"+@markup.dir)
-
+    resolved_page_name = ::File.expand_path(page_name, "#{::File::SEPARATOR}#{@markup.dir}")


Nice (helpful going forward on Windows). I guess it doesn't make sense to work with Pathname objects here, right?

Not sure. I tried putting a ::File.join here but that broke some tests. The point here is to prevent people from including /etc/password. File.expand_path here never returns a path lower in the directory tree than @markup.dir. Does Pathname have a similar method?

I'm not sure, but we could just build a Pathname object from the string that expand_path comes up with. The bigger question is whether we need something more than a string here. I guess time will tell. ;)

Right. If we want to rebuild the API for searching/retrieving objects from the repo to use Pathname, we should also do it here.

bartkamphorst · 2017-04-24T09:32:00Z

lib/gollum-lib/filter/tags.rb

      path = name
-    elsif name =~ /.+(jpg|png|gif|svg|bmp)$/i
+    elsif (file = @markup.find_file(name))
+      path = ::File.join @markup.wiki.base_path, file.path


Stylistically I'd like to see the join call have parentheses.

bartkamphorst · 2017-04-24T09:34:35Z

lib/gollum-lib/filter/tags.rb

-
-    name = parts[0].strip
-    path = parts[1] && parts[1].strip
+  def process_file_link_tag(name, path)
    if path && file = @markup.find_file(path)
      path = ::File.join @markup.wiki.base_path, file.path


Parentheses.

This methods returns HTML. Is there any way to refactor that so that gollum (and not gollum-lib) wraps path and name in the appropriate html tags?

dometto · 2017-04-27T21:04:21Z

@bartkamphorst: incorporated your stylistic suggestions, @bartkamphorst, and refactored further.

I don't see how we can easily get rid of returning HTML inside the tag filter, since the purpose of that filter (and other filters) is basically to generate HTML on top of what the markup parser generates. However, I've tried to at least cleanly separate the tag-parsing logic from the html-generation logic. Hardcoded HTML is now only present in #generate_link and #generate_image and one or two helper methods they call.

One way to get the hardcoded HTML out of gollum-lib would perhaps be to move #generate_link and #generate_image to gollum, and somehow pass them on to the tag filter as a Proc from there on? But that would mean that the filter by itself would not be fully functional 'out of the box'.

dometto · 2017-04-27T21:07:44Z

And as you suggested, process_file_link_tag and process_page_link_tag should be collapsed into a single method. But I would propose to merge this now, and to take that further step once we have refactored the Page and File classes.

dometto · 2017-04-27T21:23:53Z

Another change in behavior affected by this PR is that File links without a description now work, whereas in master they seem to result in a broken img for some reason. The latter clearly seems like a bug, so I'll add a regression test.

dometto · 2017-04-29T15:48:20Z

Re-implemented the tag skipping logic in the Markup class as you suggested, reverted the changes to the Filter class. I think this is nice and clean now, thanks for the review!

bartkamphorst · 2017-04-29T16:02:34Z

lib/gollum-lib/markup.rb

@@ -126,7 +126,7 @@ def render_default(data, format=:markdown, name='render_default.md')
      @format = format
      @name   = name

-      chain = [:YAML, :PlainText, :Emoji, :TOC, :RemoteCode, :Code, :Sanitize, :PlantUML, :Tags, :Render]
+      chain = [:YAML, :PlainText, :Emoji, :TOC, :RemoteCode, :Code, :Sanitize, :PlantUML, :Tags, :Render].reject {|filter| skip_filter?(filter)}


You mean why in the render_default method? It's used in the tests at least, so filter skipping should work there too, I thought.

Because you also call .reject on line 180.

Right, but the render and render_default methods are enitrely separate from each other, so the rejecting needs to happen twice. But I now notice that render_default is called just once in gollum-lib and gollum taken together, namely in a test. If I modify that test we can just remove this method alltogether. 👍

render_default was being used to allow use of the Markup class without a wiki. I can see some usecases for that, but I don't know we should consider them within the legitimate scope of the gollum project. Perhaps in the interest of general code sanity, we can remove it?

Go for it. 😄 People can always come knocking if they miss this functionality in 5.x.

Might be good to add to the 5.x changelog though!

Dawa Ometto added 7 commits April 23, 2017 20:40

Standardize disabling of Gollum tags per format

d2e8dea

Check tag format instead of trying to parse every tag as every kind o…

735f690

…f link

More refactoring

ecb5abe

Fix errors

93a6332

Don't implicitly assume tag links are relative to the wiki root

b91a11f

Fix image tags

03ed113

Fix include directive and tests

77fef22

bartkamphorst reviewed Apr 24, 2017

View reviewed changes

Refactor

ab06585

Dawa Ometto added 2 commits April 27, 2017 23:26

Add regression test for File links without description

76d1e33

New generic approach to skipping filters for specific formats

2b28199

dometto force-pushed the refactor_tags branch from e601ea7 to 2b28199 Compare April 27, 2017 23:37

Implement generic tag skipping in Markup instead of Filter class

d3d4fea

bartkamphorst reviewed Apr 29, 2017

View reviewed changes

Add tests. Remove Markup#render_default

e062099

dometto merged commit 745136c into gollum:gollum-lib-5.x Apr 29, 2017

dometto deleted the refactor_tags branch September 6, 2022 08:02

Refactor tags #269

Refactor tags #269

Uh oh!

Conversation

dometto commented Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dometto commented Apr 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dometto Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dometto commented Apr 27, 2017

Uh oh!

dometto commented Apr 27, 2017

Uh oh!

dometto commented Apr 27, 2017

Uh oh!

dometto commented Apr 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dometto Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dometto commented Apr 23, 2017 •

edited

Loading

dometto Apr 24, 2017 •

edited

Loading

dometto Apr 29, 2017 •

edited

Loading