opened
pmb00cs
username_0
Hi I wanted to get jekyll to automatically generate feeds for the tags I apply to my blog. I've put together this pull request, which works functionally, but doesn't build with the current rubocop settings.
I'd like some help improving the work that I have done to make it acceptable to include in this project.

I'm not primarily a ruby developer, but a Sysadmin, so pointers would be welcomed.

Strictly my use case does not need this complexity, but it annoys me that Jekyll doesn't include this functionality that is part of most popular blogging platforms.
Feed by tag
issue
created
pmb00cs
username_0
So I've written some tests, there is still some work to do on making the code pass rubocop, and it would be nice to get some help with that
comment
created
pmb00cs
username_0
So I've done some work to refactor the logic I've built to reduce the rubocop errors, but the last error I'm not sure how to correct it without breaking things badly.
comment
created
pmb00cs
username_0
So I've fixed the last of the rubocop errors, and added extra tests for when the feature I'm wanting to add is not invoked (it occured to me that I hadn't added that test and I should have) to ensure it doesn't create feeds it shouldn't do. I would still appreciate any feedback you may have.
comment
created
ashmaroli
username_1
@username_0 I've refactored the Ruby code for you since it is not a familiar language to you and therefore would have simply led to noise if I were to just leave comments behind.

That said, I've marked two areas (documentation and tests) needing changes. Please make the changes when possible.
Feel free to further improve the code if you feel the need.
comment
created
pmb00cs
username_0
@username_1 thank you for your feedback, and for your changes. Your logic looks much cleaner than mine. I've fixed the comments you have made against the tests and the README
comment
created
pmb00cs
username_0
It's been a while since I raised this pull request, and it has been waiting on a review for most of that time. I could understand if there is reluctance to accept a new feature into your project, but I would like the opportunity to try and address any such reluctance if I can. As such is there anything I can do or say, to increase the chances of getting such a review? I apologise if the wait is simply down to work load however. I do know that open source project maintainers can be short on time.
comment
created
ashmaroli
username_1
@username_0 Can you add a test-case for a scenario involving front matter key `tag`? Since Jekyll handles `tag` and `tags` automatically, there shouldn't be an issue. But it helps to be sure.
```yaml
---
tag: foobar   # value is a string instead of an array..
---

This a test document.
```
comment
created
pmb00cs
username_0
That looks like a reasonable ask. I'll see what I can do.
comment
created
pmb00cs
username_0
I've added the requested test. It's passing for me locally.
comment
created
ashmaroli
username_1
Thank you very much @username_0.

@username_2 This LGTM! as well. I'm not leaving an explicit approval here since I've made some non-trivial changes. If none of the other maintainers requests for further changes in a couple of days, feel free to merge.
comment
created
DirtyF
username_2
@username_1 yes, I would prefer a former approval from @mattr- @parkr or @benbalter as this plugin is white listed on GitHub Pages.
comment
created
ashmaroli
username_1
@username_0 I was wondering.. How about renaming `config["includes"]` to `config["only"]` so that it is more intuitive..? `includes` gives the feeling that it is to *include* a tag that is otherwise not considered... and then `config["excludes"]` to `config["except"]`..?
comment
created
pmb00cs
username_0
That looks like a sensible suggestion.
comment
created
pmb00cs
username_0
I've pushed up a change to address that @username_1 suggested to make the settings clearer on what they do.
comment
created
FREELANCEGITIT
username_3
@benbalter @username_2 @username_0 @username_1 @mojombo 
Nice work. I enjoyed reading your correspondence and am learning a lot c
From this process. Best, K
comment
created
pmb00cs
username_0
Is there any update on when the outstanding review of this pull request might happen?
comment
created
hozza
username_4
I would super love this to be merged! Perfect for connecting a Jekyll site to MailChimp for tag based email marketing! 🤩
comment
created
ashmaroli
username_1
@username_0 Will you be able to update the tests in light of Parker's review?
comment
created
pmb00cs
username_0
Hi, @username_1 I've removed the test that @parkr has raised as unneeded, and also merged the latest commits in master to ensure this pull request is fully up to date.
comment
created
DirtyF
username_2
@jekyll: merge +minor 🎉
comment