-
Notifications
You must be signed in to change notification settings - Fork 32
Add Individual Files per Entries Option #45
Conversation
062ed16
to
9f039b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some typos, and a way of testing confusion.
module Contentful | ||
# Single File Data Exporter Class | ||
# | ||
# Serializes Contentful data into a Single YAML File |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: S
ingle vs s
ingle and F
ile vs f
ile.
expect(subject.base_directory).to eq(Dir.pwd) | ||
end | ||
|
||
it 'overriden directory' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overrid
en vs overridd
en.
subject.setup_directory(expected) | ||
end | ||
|
||
it 'overridden directory' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
it 'serializes entries' do | ||
expect_any_instance_of(::Jekyll::Contentful::Serializer).to receive(:serialize) { {} } | ||
|
||
subject.run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that style of testing only validates, that no error/exception is thrown. Do you mind adding some more precise validations? Aka that the right entry got serialized to the right result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are tests for the serializers just doing that, this here tests that the logic is followed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I do not see how you test that the logic is followed. The only thing I can see is validating that nothing throws, unless I'm overlooking something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm testing that when the exporter is running it's calling the serializer.
|
||
expected_directory_path = File.join(Dir.pwd, '_data', 'contentful', 'spaces', 'foo', 'bar_ct') | ||
expect(FileUtils).to receive(:mkdir_p).with(expected_directory_path) | ||
expect(File).to receive(:open).with(File.join(expected_directory_path, 'bar.yaml'), 'w') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does not test, what the name suggest, right? I was expecting it to check the existence of all the files created, and not just one. Or I'm miss reading the test name ... :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a single entry, so it just checks for a single file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, I'm confused by the naming ... ;) Should we call it creates a file or rather have the test create multiple files and validate that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the setup for the subject
I'm sending it a list with a single entry, therefore creating a single file for that entry is the expected behaviour. As it is creating a file for a specific entry with a path that looks like {base_dir}/_data/contentful/spaces/{space_alias}/{content_type_id}/{entry_id}.yaml
. While the single_file_data_exporter
creates a file for the space instead (and all content types and entries are inside that file).
9f039b8
to
11b0b34
Compare
11b0b34
to
3ff0c3c
Compare
Fixes #23
Fixes #10
Fixes #25