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

Improve flexibility of Atom feed generator #1301

Merged
merged 2 commits into from Feb 17, 2018

Conversation

Projects
None yet
2 participants
@iay
Copy link
Contributor

commented Jan 24, 2018

Allow overriding <id> and <link rel="alternate"> elements.

Detailed description

Adds two new parameters, :alt_link and :id, to the atom_feed method. These allow the overriding of the <id> and <link rel="alternate"> elements of the generated Atom feed.

Overriding these can result in greater compatibility and also allow for the possibility of multiple feeds per site.

I have manually tested that if you don't supply these new parameters, you get the same output as before. As mentioned in the comments on #1300, I think there's a separate issue that the default for :alt_link should probably be something like "/" instead of the base URL, but that seems like a separate issue to this enhancement.

To do

(Include the to-do list for this PR to be finished here.)

  • Tests
  • Documentation
  • Feature flags

Related issues

Would address #1300.

@iay

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

  1. I might take a look this weekend at adding some tests for this, it's about time I learned TDD in Ruby. What's the best reference for the testing framework you're using?

  2. I'm not sure how to interpret the CI failures, are they on my side somehow?

  3. Do you want me to rebase this against the current HEAD?

@iay iay force-pushed the iay:iay-1300-atom-flex branch from 3e65f7c to 2e282e9 Jan 26, 2018

@iay

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

I think I rebased the original commit; CI still failing.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

@iay The only remaining error that I can see is

 1) Failure:
Nanoc::Helpers::BloggingTest#test_atom_feed_without_base_url [/home/travis/build/nanoc/nanoc/nanoc/test/helpers/test_blogging.rb:215]:
[Nanoc::Int::Errors::GenericTrivial] exception expected, not
Class: <NoMethodError>
Message: <"undefined method `+' for nil:NilClass">
---Backtrace---
/home/travis/build/nanoc/nanoc/nanoc/lib/nanoc/helpers/blogging.rb:196:in `atom_feed'
/home/travis/build/nanoc/nanoc/nanoc/test/helpers/test_blogging.rb:216:in `block in test_atom_feed_without_base_url'
---------------

… which I believe is related to the change made

@iay

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2018

Thanks; I need to fix that but I'll do it in conjunction with learning RSpec and adding some positive tests for the new code. Should get to it over the next couple of weeks (there's really not hurry on this one).

@iay iay force-pushed the iay:iay-1300-atom-flex branch 2 times, most recently from 1018ca1 to b81dc43 Feb 3, 2018

Improve flexibility of Atom feed generator
Adds two new parameters, :alt_link and :id, to the atom_feed method.
These allow the overriding of the <id> and <link rel="alternate">
elements of the generated Atom feed.

Overriding these can result in greater compatibility and also allow for
the possibility of multiple feeds per site.

@iay iay force-pushed the iay:iay-1300-atom-flex branch from b81dc43 to ca22ae3 Feb 3, 2018

@iay

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2018

Reworked to avoid the exception, rebased on latest.

I'm trying to figure out how I would make tests for this change. Am I right in saying that you have both RSpec based tests under nanoc/spec and also more conventional unit tests under nanoc/test?

It looks like there are no tests for the actual XML build in the RSpec section of the repository, so I'm guessing that it might be easiest to add some tests to the end of nanoc/test/helpers/test_blogging.rb and just match expected content within the XML for the item as a string, as for example is being done in test_atom_feed_with_xml_base. Does that sound right?

@iay

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2018

A related question would be whether there was an XML parser available in that test context; it seems like a test that re-parsed the resulting string and then dug into the structure would be less brittle than one involving just regexes.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

Am I right in saying that you have both RSpec based tests under nanoc/spec and also more conventional unit tests under nanoc/test?

Correct. The test/ directory is older; eventually I hope to have everything migrated to spec/, but there’s no rush.

I'm guessing that it might be easiest to add some tests to the end of nanoc/test/helpers/test_blogging.rb and just match expected content within the XML for the item as a string, as for example is being done in test_atom_feed_with_xml_base. Does that sound right?

Yes!

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Feb 7, 2018

A related question would be whether there was an XML parser available in that test context; it seems like a test that re-parsed the resulting string and then dug into the structure would be less brittle than one involving just regexes.

Definitely. Nokogiri likely will be available, so you could do something similar to what’s being done in test_xml_sitemap.rb:

      doc = Nokogiri::XML(res)
      urlsets = doc.css('> urlset')
      assert_equal 1, urlsets.size
      urls = urlsets.css('> url')
      assert_equal 4, urls.size
      assert_equal 'http://example.com/item-one/a/',   urls[0].css('> loc').inner_text
      assert_equal 'http://example.com/item-one/b/',   urls[1].css('> loc').inner_text
      assert_equal 'http://example.com/item-three/a/', urls[2].css('> loc').inner_text
      assert_equal 'http://example.com/item-three/b/', urls[3].css('> loc').inner_text
      #
@iay

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2018

Added unit tests.

So now I know how to use Nokogiri, I guess. Is there anything else you need here?

Also, is there a way to manually run just a single file of tests in isolation? Took a while to get started on this because it takes a while to run everything just to find out that I've written my XPath expressions incorrectly 😄

@ddfreyne ddfreyne merged commit c83ec7a into nanoc:master Feb 17, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 9556a6e...719f2a4
Details
codecov/project 98.49% (+<.01%) compared to 9556a6e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ddfreyne

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

Also, is there a way to manually run just a single file of tests in isolation?

Yeah, but I just realised that this approach isn’t documented.

From within the nanoc subdirectory, either

  • (for something in test) run bundle exec m test/path/to/test_something.rb
  • (for something in spec) run bundle exec rspec spec/path/to/something_spec.rb
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.