Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Jekyll::Cache #7532

Merged
merged 10 commits into from May 1, 2019

Conversation

Projects
None yet
4 participants
@ashmaroli
Copy link
Member

commented Feb 15, 2019

Summary

  • Replace class variables with instance variables in the singleton class because class variables can be permanently altered by sub classes.
  • Group all class methods together.
  • Add some Minitests and some additional Cucumber steps

@ashmaroli ashmaroli added the refactor label Feb 15, 2019

@ashmaroli ashmaroli added this to the 4.0 milestone Feb 15, 2019

@ashmaroli ashmaroli requested a review from pathawks Feb 15, 2019

@update-docs

This comment was marked as off-topic.

Copy link

commented Feb 15, 2019

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update some of our documentation based on your changes.

@DirtyF
Copy link
Member

left a comment

👍 once the tests pass

@DirtyF

DirtyF approved these changes Mar 15, 2019

@mattr-

mattr- approved these changes Apr 30, 2019

Copy link
Member

left a comment

Looking good. Just one small nit that you can feel free to ignore.


path = path_to(hash(key))
File.delete(path)
File.delete(path_to(hash(key))) if disk_cache_enabled?

This comment has been minimized.

Copy link
@mattr-

mattr- Apr 30, 2019

Member

This might just be personal preference, but I found the original version easier to read and reason about.

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Apr 30, 2019

Author Member

Rationales for this change:

  • Remove avoidable local variable allocation
  def delete(key)
    @cache.delete(key)
    return unless disk_cache_enabled?

-   path = path_to(hash(key))
-   File.delete(path)
+   File.delete(path_to(hash(key)))
  end
  • Remove guard clause (cosmetic change)
  def delete(key)
    @cache.delete(key)
-   return unless disk_cache_enabled?
-
-   File.delete(path_to(hash(key)))
+   File.delete(path_to(hash(key))) if disk_cache_enabled
  end

Can I just undo the cosmetic change?

@DirtyF

This comment has been minimized.

Copy link
Member

commented May 1, 2019

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 3e8c37b into jekyll:master May 1, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

jekyllbot added a commit that referenced this pull request May 1, 2019

@ashmaroli ashmaroli deleted the ashmaroli:refactor-cache branch May 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.