-
Notifications
You must be signed in to change notification settings - Fork 203
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
Feed by tag #264
Feed by tag #264
Conversation
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 |
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. |
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. |
@pmb00cs 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. |
@ashmaroli 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 |
I've pushed up a change to address that @ashmaroli suggested to make the settings clearer on what they 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.
Thanks for adding a test for security concern Ashwin! 💪 🔒
Is there any update on when the outstanding review of this pull request might happen? |
I would super love this to be merged! Any update on this @benbalter? It would be perfect for connecting a Jekyll site to MailChimp for tag based email marketing! 🤩 |
This comment has been minimized.
This comment has been minimized.
Hi, @ashmaroli 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. |
@jekyll: merge +minor 🎉 |
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.