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

Feed by tag #264

Merged
merged 31 commits into from Jun 22, 2020
Merged

Feed by tag #264

merged 31 commits into from Jun 22, 2020

Conversation

pmb00cs
Copy link
Contributor

@pmb00cs pmb00cs commented Feb 17, 2019

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.

@pmb00cs
Copy link
Contributor Author

pmb00cs commented Feb 18, 2019

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

@pmb00cs
Copy link
Contributor Author

pmb00cs commented Feb 18, 2019

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.

@pmb00cs
Copy link
Contributor Author

pmb00cs commented Feb 18, 2019

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.

README.md Show resolved Hide resolved
spec/jekyll-feed_spec.rb Outdated Show resolved Hide resolved
@ashmaroli
Copy link
Member

@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.
Feel free to further improve the code if you feel the need.

@pmb00cs
Copy link
Contributor Author

pmb00cs commented Feb 19, 2019

@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

@ashmaroli ashmaroli dismissed their stale review February 20, 2019 06:42

Concerns addressed

@pmb00cs
Copy link
Contributor Author

pmb00cs commented Jan 15, 2020

I've pushed up a change to address that @ashmaroli suggested to make the settings clearer on what they do.

Copy link
Member

@DirtyF DirtyF left a 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! 💪 🔒

@pmb00cs
Copy link
Contributor Author

pmb00cs commented May 15, 2020

Is there any update on when the outstanding review of this pull request might happen?

@hozza
Copy link

hozza commented Jun 18, 2020

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! 🤩

@ashmaroli ashmaroli requested review from parkr and pathawks and removed request for benbalter June 21, 2020 06:38
spec/jekyll-feed_spec.rb Outdated Show resolved Hide resolved
@ashmaroli

This comment has been minimized.

@DirtyF DirtyF added this to the Next minor version milestone Jun 22, 2020
@pmb00cs
Copy link
Contributor Author

pmb00cs commented Jun 22, 2020

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.

@DirtyF
Copy link
Member

DirtyF commented Jun 22, 2020

@jekyll: merge +minor 🎉

@jekyllbot jekyllbot merged commit b7f758b into jekyll:master Jun 22, 2020
jekyllbot added a commit that referenced this pull request Jun 22, 2020
@pmb00cs pmb00cs deleted the feed_by_tag branch June 22, 2020 16:30
@jekyll jekyll locked and limited conversation to collaborators Jun 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants