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

Use default Rake tasks and scripts #55

Merged
merged 3 commits into from
Feb 7, 2018
Merged

Use default Rake tasks and scripts #55

merged 3 commits into from
Feb 7, 2018

Conversation

jekyllbot
Copy link
Contributor

@jekyllbot jekyllbot commented Feb 7, 2018

This PR intent is to normalize the documentation for testing and releasing Jekyll plugins.

https://jekyllrb.com/docs/maintaining/releasing-a-new-version/#for-non-core-gems

@DirtyF DirtyF requested review from benbalter and a team February 7, 2018 19:42
@@ -1,5 +1,7 @@
#! /bin/sh

bundle exec rspec
bundle exec rubocop -S -D
gem build jekyll-mentions.gemspec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not want to test that dependencies resolve and that the Gem builds?

Copy link
Member

@DirtyF DirtyF Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, default bundler rake task bundle exec rake build should take care of that.

@@ -1,5 +1,5 @@
# frozen_string_literal: true

module JekyllMentions
VERSION = "1.2.0"
VERSION = "1.2.0".freeze
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be necessary because of frozen_string_literal: true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah but Rubcop complains for older Rubies and we don't want to target Ruby 2.3+ for now 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's ignore this cop. This is nonsensical.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's what Jekyll does, so 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we should fix that in Jekyll's config.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By adding an explicit freeze call, the constant is guaranteed frozen on all supported Rubies instead of just 2.3+.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So let’s drop the magic comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) why is this bothering you so much..?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m grumpy 😜
You’re right; it doesn’t matter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry @pathawks , I promise we'll get rid of all those useless .freeze at the end of march when we stop supporting EOL rubies :)

@DirtyF
Copy link
Member

DirtyF commented Feb 7, 2018

@jekyllbot: :shipit: +dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants