Skip to content
This repository has been archived by the owner on Jul 27, 2024. It is now read-only.

Refactor InMemory Stuff -> {InMemory,FileSystem}Storage #148

Merged
merged 3 commits into from
Feb 10, 2021

Conversation

charlespwd
Copy link
Contributor

@charlespwd charlespwd commented Feb 10, 2021

Follow up on #145

From

class InMemoryTemplate < Template; end
class InMemoryJsonFile < JsonFile; end
class InMemoryTheme < Theme; end

to

class Storage; end
class InMemoryStorage < Storage; end
class FileSystemStorage < Storage; end

class Theme
  def initialize(storage); end
end

class Template
  def initialize(name, storage); end
end

class JsonFile
  def initialize(name, storage); end
end

Copy link
Contributor

@macournoyer macournoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful refactoring 👏

Just a few optimization ideas.

def lines
# Retain trailing newline character
@lines ||= source.split("\n", -1)
end

# Not entirely obvious but lines is mutable, corrections are to be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead we should just reset @lines in write, this way lines will always be the correct method to call:

def write
  # ...
  @lines = nil
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand this one. The intention behind this method was to do a check on source != updated_source before writing. And to use it in tests rather than the path.read we had in test_helper.

@charlespwd charlespwd force-pushed the feature/in-memory-theme-check branch 2 times, most recently from a3c783a to a8bb556 Compare February 10, 2021 16:59
storage = make_storage(files)
@template = ThemeCheck::Template.new(
"templates/index.liquid",
storage
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're back at one. Can probably inline a couple of those vars.

@charlespwd charlespwd force-pushed the feature/in-memory-theme-check branch from 77a1178 to cc68206 Compare February 10, 2021 18:41
@charlespwd charlespwd force-pushed the feature/in-memory-theme-check branch from cc68206 to 90799f5 Compare February 10, 2021 18:42
@charlespwd charlespwd merged commit 59e8a1e into master Feb 10, 2021
@macournoyer macournoyer temporarily deployed to rubygems February 16, 2021 14:44 Inactive
@charlespwd charlespwd deleted the feature/in-memory-theme-check branch February 18, 2021 21:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants