Consistency
Sometimes you use snake_case, sometimes you use camelCase for methods and local variables. Sometime you use single-quoted strings and sometimes you use double-quoted strings.
You should choose one style and stick with it. If you are editing some existing code, you should adapt your style to be the same as the existing code. If you are part of a team, you should adapt your style to match the rest of the team.
Most communities have developed standardized community style guides. In Ruby, there are multiple such style guides. They all agree on the basics (e.g. indentation is 2 spaces), but they might disagree on more specific points (single quotes or double quotes).
In general, if you use two different ways to write the exact same thing, the reader will think that you want to convey a message with that. So, you should only use two different ways of writing the same thing IFF you actually want to convey some extra information.
For example, some people always use parentheses for defining and calling purely functional side-effect free methods, and never use parentheses for defining and calling impure methods. That is a good reason to use two different styles (parentheses and no parentheses) for doing the same thing (defining methods).
Single-quoted strings
If you don't use string interpolation, it is helpful if you use single quotes for your strings. That way, it is immediately obvious that no string interpolation is taking place.
require "jekyll"
should instead be
require 'jekyll'
Note that on the very next line, you use
require 'zip'
That is inconsistent and it trips up the reader, because they are left wondering what the difference is between zip and jekyll. Why did you choose to enclose one in single quotes and one in double quotes? What does that mean? What are you trying to tell us?
Note that it is perfectly fine to use double quoted strings if you otherwise needed to use escapes, e.g. if you had some something like
puts "Philipp's Jekyll plugin"
that reads much better than
puts 'Philipp\'s Jekyll plugin'
So in this case, double quotes would be preferred.
Frozen string literals
Immutable data structures and purely functional code are always preferred, unless mutability and side-effects are required for clarity or performance. In Ruby, strings are always mutable, but there is a magic comment you can add to your files (also available as a command-line option for the Ruby engine), which will automatically make all literal strings immutable:
# frozen_string_literal: true
It is generally preferred to add this comment to all your files.
Freeze objects assigned to constants
As mentioned above, you should prefer as much as possible to use purely functional code, immutable data, and immutable bindings.
Here, you assign a mutable value to a constant:
VARIABLE_SYNTAX = %r![^{]*(\{\{\s*[\w\-\.]+\s*(\|.*)?\}\}[^\s{}]*)!mx
This could be somewhat confusing because while the object is assigned to a constant, the object itself can still be changed. It is better to freeze objects assigned to constants:
VARIABLE_SYNTAX = %r![^{]*(\{\{\s*[\w\-\.]+\s*(\|.*)?\}\}[^\s{}]*)!mx.freeze
Prefer / for Regexp literals
It is generally preferred to use / to delimit Regexp literals unless you need to use / within your Regexp literal:
VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w\-\.]+\s*(\|.*)?\}\}[^\s{}]*)/mx.freeze
Superfluous escape in the Regexp literal
There is no need to escape the . character in a character class: it has no special meaning inside a character class.
VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w\-.]+\s*(\|.*)?\}\}[^\s{}]*)/mx.freeze
Also, you don't need to escape - in a character class if it is the first or last character in the class:
VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w.-]+\s*(\|.*)?\}\}[^\s{}]*)/mx.freeze
Superfluous option in the Regexp literal
You are using the x eXtended mode option, which allows you to write the Regexp in multiple lines and add comments to it … but you are not actually doing any of that. So, it is just extra fluff that can be removed
VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w.-]+\s*(\|.*)?\}\}[^\s{}]*)/m.freeze
Prefer predicate methods over relational operators
You should prefer "speaking" predicate methods such as Numeric#negative? over relational operators such as < 0.
So, this:
elsif file.strip.length > 0
should be
elsif file.strip.length.positive?
Prefer predicate methods over explicit length checks
As mentioned above, you should prefer predicates over relational operators. But in this specific case, we can even do one better, we can use String#empty?:
elsif file.strip.length.positive?
can just be
elsif !file.strip.empty?
Space after # in comments
There should be one space after the # in a comment:
#~ gem 'rubyzip', '~>2.3.0'
should be
# ~ gem 'rubyzip', '~>2.3.0'
Actually, it looks like you just commented out a line of code. You can just delete it. If you want it back, that's what Version Control Systems are for.
No empty line after opening keyword
There should be no empty line after the opening module, class, def, do, while, until, if, unless, case, etc.
Which means there should be no empty line here:
class ZipBundlerTag < Liquid::Tag
VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w.-]+\s*(\|.*)?\}\}[^\s{}]*)/m.freeze
it should just be
class ZipBundlerTag < Liquid::Tag
VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w.-]+\s*(\|.*)?\}\}[^\s{}]*)/m.freeze
Modifier if
If the body of a conditional expression is only a single expression, you can use the modifier form of the conditional.
For example, you can use that here:
if files.length < 2
abort 'zip tag must be called with at least two files'
end
could be
abort 'zip tag must be called with at least two files' if files.length < 2
Use of deprecated / removed methods
File::exists? is deprecated, and has been for many years. Its documentation literally reads, in its entirety:
Deprecated method. Don't use.
In fact, in current versions of Ruby, the method actually does not exist anymore. This means that in current versions of Ruby, your code will fail with a NoMethodError exception!
You should use File::exist? instead.
Prefer string interpolation over concatenation
site.source + '/' + cacheFolder
should be
"#{site.source}/{cacheFolder}"
Although in this particular case, since you are actually constructing a path, you could also use one of the methods dedicated to that purpose, like File::join.
You already use similar methods for finding the name of the directory and the base name.
Linting
You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.
Rubocop was able to detect almost all of the style violations I pointed out above (plus some more), and also was able to autocorrect most of the ones it detected.
Let me repeat that: I have just spent two pages pointing out how to correct tons of stuff that you can actually correct within milliseconds at the push of a button. I have set up my editor such that it automatically runs Rubocop with auto-fix as soon as I hit "save".
In particular, running Rubocop on your code, it detects 29 offenses, of which it can automatically correct 19.
Here's what the result of the auto-fix looks like:
# frozen_string_literal: true
# Copyright 2021 by Philipp Hasper
# MIT License
# https://github.com/PhilLab/jekyll-zip-bundler
require 'jekyll'
require 'zip'
# ~ gem 'rubyzip', '~>2.3.0'
module Jekyll
# Valid syntax:
# {% zip archiveToCreate.zip file1.txt file2.txt %}
# {% zip archiveToCreate.zip file1.txt file2.txt %}
# {% zip archiveToCreate.zip file1.txt folder/file2.txt 'file with spaces.txt' %}
# {% zip {{ variableName }} file1.txt 'folder/file with spaces.txt' {{ otherVariableName }} %}
# {% zip {{ variableName }} {{ VariableContainingAList }} %}
class ZipBundlerTag < Liquid::Tag
VARIABLE_SYNTAX = /[^{]*(\{\{\s*[\w\-.]+\s*(\|.*)?\}\}[^\s{}]*)/mx.freeze
def initialize(tagName, markup, tokens)
super
# Split by spaces but only if the text following contains an even number of '
# Based on https://stackoverflow.com/a/11566264
# Extended to also not split between the curly brackets of Liquid
@files = markup.strip.split(/\s(?=(?:[^'}]|'[^']*'|{{[^}]*}})*$)/)
end
def render(context)
files = []
# Resolve the given parameters to a file list
@files.each do |file|
matched = file.strip.match(VARIABLE_SYNTAX)
if matched
# This is a variable. Look it up.
resolved = context[file]
if resolved.respond_to?(:each)
# This is a collection. Flatten it before appending
resolved.each do |file|
files.push(file)
end
else
files.push(resolved)
end
elsif file.strip.length.positive?
files.push(file.strip)
end
end
# First file is the target zip archive path
abort 'zip tag must be called with at least two files' if files.length < 2
# Generate the file in the cache folder
cacheFolder = '.jekyll-cache/zip_bundler/'
zipfile_path = cacheFolder + files[0]
FileUtils.makedirs(File.dirname(zipfile_path))
files_to_zip = files[1..-1]
# Create the archive. Delete file, if it already exists
File.delete(zipfile_path) if File.exist?(zipfile_path)
Zip::File.open(zipfile_path, Zip::File::CREATE) do |zipfile|
files_to_zip.each do |file|
# Two arguments:
# - The name of the file as it will appear in the archive
# - The original file, including the path to find it
zipfile.add(File.basename(file), file)
end
end
puts "Created archive #{zipfile_path}"
# Add the archive to the site's static files
site = context.registers[:site]
site.static_files << Jekyll::StaticFile.new(site, "#{site.source}/#{cacheFolder}", File.dirname(files[0]),
File.basename(zipfile_path))
# No rendered output
''
end
end
end
Liquid::Template.register_tag('zip', Jekyll::ZipBundlerTag)
And here are the offenses that Rubocop could not automatically correct:
Inspecting 1 file
W
Offenses:
zip_bundler.rb:21:20: C: Naming/MethodParameterName: Only use lowercase characters for method parameter.
def initialize(tagName, markup, tokens)
^^^^^^^
zip_bundler.rb:21:20: C: Naming/VariableName: Use snake_case for variable names.
def initialize(tagName, markup, tokens)
^^^^^^^
zip_bundler.rb:29:5: C: Metrics/AbcSize: Assignment Branch Condition size for render is too high. [<11, 36, 10> 38.95/17]
def render(context) ...
^^^^^^^^^^^^^^^^^^^
zip_bundler.rb:29:5: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for render is too high. [9/7]
def render(context) ...
^^^^^^^^^^^^^^^^^^^
zip_bundler.rb:29:5: C: Metrics/MethodLength: Method has too many lines. [32/10]
def render(context) ...
^^^^^^^^^^^^^^^^^^^
zip_bundler.rb:29:5: C: Metrics/PerceivedComplexity: Perceived complexity for render is too high. [11/8]
def render(context) ...
^^^^^^^^^^^^^^^^^^^
zip_bundler.rb:39:31: W: Lint/ShadowingOuterLocalVariable: Shadowing outer local variable - file.
resolved.each do |file|
^^^^
zip_bundler.rb:53:7: C: Naming/VariableName: Use snake_case for variable names.
cacheFolder = '.jekyll-cache/zip_bundler/'
^^^^^^^^^^^
zip_bundler.rb:54:22: C: Naming/VariableName: Use snake_case for variable names.
zipfile_path = cacheFolder + files[0]
^^^^^^^^^^^
zip_bundler.rb:73:75: C: Naming/VariableName: Use snake_case for variable names.
site.static_files << Jekyll::StaticFile.new(site, "#{site.source}/#{cacheFolder}", File.dirname(files[0]),
^^^^^^^^^^^
1 file inspected, 10 offenses detected
It is a good idea to set up your tools such that the linter is automatically run when you paste code, edit code, save code, commit code, or build your project, and that passing the linter is a criterium for your CI pipeline.
In my editor, I actually have multiple linters and static analyzers integrated so that they automatically always analyze my code, and also as much as possible automatically fix it while I am typing. This can sometimes be annoying (e.g. I get 76 notices for your original code, lots of which are duplicates because several different tools report the same problem), but it is in general tremendously helpful. It can be overwhelming when you open a large piece of code for the first time and you get dozens or hundreds of notices, but if you start a new project, then you can write your code in a way that you never get a notice, and your code will usually be better for it.
However, even by simply hitting "Save", my editor applies a series of automatic fixes which brings the number of notices down to 48. Running Rubocop as described above, further reduces this to 38, and as mentioned, lots of these are duplicates because I have multiple different linters and analyzers configured. I would say about 16 are unique.
Use snake_case for local variables
Methods, local variables, instance variables, class variables, global variables, and parameters should use snake_case naming convention. You are jumping back and fort between camelCase and snake_case, for example here:
cacheFolder = '.jekyll-cache/zip_bundler/'
zipfile_path = cacheFolder + files[0]
This should be
cache_folder = '.jekyll-cache/zip_bundler/'
zipfile_path = cache_folder + files[0]
With a good editor or IDE, you should be able to fix this fairly easily with the Rename Variable Refactoring.
This brings the number of Rubocop offenses down to 5 and the number of notices in my editor down to 26.
The hard stuff
Note that all we did so far was either done automatically for us by the editor or Rubocop's auto-correct feature, or we were blindly following instructions such as renaming variables. We did not yet have to think at all.
However, if we look at the remaining notices we get from Rubocop and the other linters and analyzers, it is clear that we will have to apply some brains to get rid of them:
Duplicate code
Some of these are still fairly easy to get rid of, in particular the ones about duplicate code.
For example, files[0] is really the target. You even have a comment that says so:
# First file is the target zip archive path
So, let's just make that clear:
target = files.first
And now we can replace all occurrences of files[0] with target, which not only gets rid of the notices in the editor, but also makes the code more intention-revealing.
The same applies to the file.strip. We could simply assign something like stripped_file = file.strip and replace all the mentions of file.strip with stripped_file. But in my opinion, it would be even better to fix the problem at the source, and make sure that the @files variable only contains pre-stripped files.
So, we change the initialize method to
@files = markup.strip.split(/\s(?=(?:[^'}]|'[^']*'|{{[^}]*}})*$)/).map(&:strip)
Note: it might also be possible to tweak the Regexp so that there is nothing to strip in the first place.
The same applies to the file.empty? check. We can either remove empty strings right here:
@files = markup.strip.split(/\s(?=(?:[^'}]|'[^']*'|{{[^}]*}})*$)/).map(&:strip).reject(&:empty?)
or maybe tweak the Regexp in order to not produce empty strings in the first place.
Better design
Unfortunately, getting rid of most of the other notices and offenses requires a level of domain knowledge of Liquid that I do not have.
For example, I don't know what kinds of collections can appear at this point:
if resolved.respond_to?(:each)
# This is a collection. Flatten it before appending
resolved.each do |file|
files.push(file)
end
If these collections respond to to_ary, then we can replace the whole loop with a map + flatten, something like this:
target, files = @files.map do |file|
next file unless file.match(VARIABLE_SYNTAX)
# This is a variable. Look it up.
context[file]
end.flatten
Splitting code
You have already put in some comments explaining the individual steps. As a very general rule, whenever you write a comment, you should take that as an opportunity to take a step back and think about why you are writing that comment in the first place.
In particular,
- if you are writing the comment because the code is too complex, then make the code simpler and the comment is no longer needed
- if you are writing the comment to explain what something is, then give it a better name (or give it a name in the first place if it doesn't have one) and the comment is no longer needed
- if you are writing the comment to point out logical breaks in the code, then split the code at that point and the comment is no longer needed
- if you are writing the comment to explain what the code does, then really the code should explain what the code does instead, and the comment should not be needed
The main reason to have a comment is to explain why a piece of code does something in a certain way that may be non-obvious.
So, looking at the comments that delineate the separate steps, maybe we can split the code at those points and actually make each step a separate method.
It would look somewhat like this:
def render(context)
# First file is the target zip archive path
target, files = resolve_parameters(context)
abort 'zip tag must be called with at least two files' if files.empty?
zipfile_path = CACHE_FOLDER + target
klass = self.class
klass.create_directory(zipfile_path)
klass.create_archive(files, zipfile_path)
klass.register_files(context, target, zipfile_path)
# No rendered output
''
end
private
def resolve_parameters(context)
target, files = @files.map do |file|
next file unless file.match(VARIABLE_SYNTAX)
# This is a variable. Look it up.
context[file]
end
[target, files]
end
private_class_method def self.create_directory(zipfile_path)
FileUtils.makedirs(File.dirname(zipfile_path))
end
private_class_method def self.create_archive(files, zipfile_path)
File.delete(files, zipfile_path) if File.exist?(zipfile_path)
Zip::File.open(zipfile_path, Zip::File::CREATE) do |zipfile|
files.each do |file|
# Two arguments:
# - The name of the file as it will appear in the archive
# - The original file, including the path to find it
zipfile.add(File.basename(file), file)
end
end
puts "Created archive #{zipfile_path}"
end
private_class_method def self.register_files(context, target, zipfile_path)
# Add the archive to the site's static files
site = context.registers[:site]
site.static_files << Jekyll::StaticFile.new(site, File.join(site.source, CACHE_FOLDER), File.dirname(target),
File.basename(zipfile_path))
end
As mentioned above, I don't have enough domain knowledge in Jekyll or Liquid to make any suggestions about a fundamentally better design. Maybe there are some helper methods that could be used. For example, I cannot imagine that you have to parse variables yourself. (But I was wrong, the documentation actually explicitly says that you have to do that.)
