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

FileSystemUnified data source: added configurable dirs #412

Merged
merged 1 commit into from Apr 3, 2014

Conversation

Projects
None yet
2 participants
@gpakosz
Member

gpakosz commented Apr 1, 2014

I added configurable prefixes to FileSystemUnified data source:

data_sources:
  - type:           filesystem_unified
    items_prefix:   items
    layouts_prefix: layouts
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Apr 1, 2014

Member

For consistency with Filesystem in nanoc 4.0, I’d change the names slightly:

  • content_dir for items
  • layouts_dir for layouts

I’d also like to have methods content_dir_name and layouts_dir_name that do @config.fetch(:content_dir, 'content') and @config.fetch(:layouts_dir, 'layouts'), respectively.

(The Filesystem data source in nanoc 4.0 is not that nice yet. It doesn’t have content_dir_name and layouts_dir_name, for instance. I also noticed it uses %w( content layouts ) which is not parameterised, oops.)

Member

ddfreyne commented Apr 1, 2014

For consistency with Filesystem in nanoc 4.0, I’d change the names slightly:

  • content_dir for items
  • layouts_dir for layouts

I’d also like to have methods content_dir_name and layouts_dir_name that do @config.fetch(:content_dir, 'content') and @config.fetch(:layouts_dir, 'layouts'), respectively.

(The Filesystem data source in nanoc 4.0 is not that nice yet. It doesn’t have content_dir_name and layouts_dir_name, for instance. I also noticed it uses %w( content layouts ) which is not parameterised, oops.)

# prefix: assets
# data_sources:
# - type: static
# prefix: assets

This comment has been minimized.

@ddfreyne

ddfreyne Apr 1, 2014

Member

Thanks for spotting this :)

@ddfreyne

ddfreyne Apr 1, 2014

Member

Thanks for spotting this :)

@gpakosz

This comment has been minimized.

Show comment
Hide comment
@gpakosz

gpakosz Apr 2, 2014

Member

I'm about to replace this pull request with a commit that does what you asked for.

But some tests fail because @config returns nil. Should it be @config.fetch(:content_dir, 'content') rescue 'content' and @config.fetch(:layouts_dir, 'layouts') rescue 'layouts'?

Member

gpakosz commented Apr 2, 2014

I'm about to replace this pull request with a commit that does what you asked for.

But some tests fail because @config returns nil. Should it be @config.fetch(:content_dir, 'content') rescue 'content' and @config.fetch(:layouts_dir, 'layouts') rescue 'layouts'?

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Apr 2, 2014

Member

Hmm, @config should ideally never be nil… can you fix the code so that it is never nil?

Alternative, you could have def config ; @config || [] ; end.

Member

ddfreyne commented Apr 2, 2014

Hmm, @config should ideally never be nil… can you fix the code so that it is never nil?

Alternative, you could have def config ; @config || [] ; end.

@gpakosz

This comment has been minimized.

Show comment
Hide comment
@gpakosz

gpakosz Apr 2, 2014

Member

I started modifying the tests so that @config isn't nil. Which is kinda convoluted because lots of tests just assume it's ok to pass nil for config.

Somehow, it seems to me it's quite acceptable to change https://github.com/nanoc/nanoc/blob/master/lib/nanoc/base/source_data/data_source.rb#L61 to@config = config || {}.

What do you think?

PS: @config or config? FilesystemUnified uses the former while Static (which is more recent) uses the later.

Member

gpakosz commented Apr 2, 2014

I started modifying the tests so that @config isn't nil. Which is kinda convoluted because lots of tests just assume it's ok to pass nil for config.

Somehow, it seems to me it's quite acceptable to change https://github.com/nanoc/nanoc/blob/master/lib/nanoc/base/source_data/data_source.rb#L61 to@config = config || {}.

What do you think?

PS: @config or config? FilesystemUnified uses the former while Static (which is more recent) uses the later.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Apr 2, 2014

Member

I’m OK with @config = config || {}.

I’d like to only use @config in the initializer, and config elsewhere in the file.

Member

ddfreyne commented Apr 2, 2014

I’m OK with @config = config || {}.

I’d like to only use @config in the initializer, and config elsewhere in the file.

@gpakosz gpakosz changed the title from FileSystemUnified data source: added configurable prefixes to FileSystemUnified data source: added configurable dirs Apr 2, 2014

FileSystemUnified data source: added configurable dirs for items and …
…layouts

    content_dir for items
    layouts_dir for layouts
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Apr 3, 2014

Member

Looks good, thanks!

Member

ddfreyne commented Apr 3, 2014

Looks good, thanks!

ddfreyne added a commit that referenced this pull request Apr 3, 2014

Merge pull request #412 from gpakosz/filesystem_unified-prefixes
FileSystemUnified data source: added configurable dirs

@ddfreyne ddfreyne merged commit 3ea38fe into nanoc:master Apr 3, 2014

1 check passed

default The Travis CI build passed
Details

@gpakosz gpakosz deleted the gpakosz:filesystem_unified-prefixes branch Jul 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment