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

Change cucumber tests to not write posts with timezones #1124

Merged
merged 3 commits into from May 23, 2013

Conversation

Projects
None yet
4 participants
@jpiasetz
Contributor

jpiasetz commented May 18, 2013

Changes jekyll_steps so that instead of writing the full DateTime it just writes Y-m-d H:m. That should avoid all the timezone issues. Should fix #1090

@parkr

This comment has been minimized.

Member

parkr commented May 18, 2013

Hm, looks like that change causes an error in every version of ruby we test against.

@jpiasetz

This comment has been minimized.

Contributor

jpiasetz commented May 18, 2013

Sorry should have been paying more attention. Now all green except for 2.0. Locally it's passing on 2.0 for me though so might take a bit to sort out.

@jpiasetz

This comment has been minimized.

Contributor

jpiasetz commented May 21, 2013

@parkr I'm confused by the last failure. Locally it would pass and I got my repo to be all green on travis even though the last commit wouldn't. I think it's good to merge but might be good to give it a review just to be safe.

@mattr-

This comment has been minimized.

Member

mattr- commented May 21, 2013

👍 from me.

@parkr

This comment has been minimized.

Member

parkr commented May 21, 2013

Is there any way we can shore up the duplication here and (separately) defer to a separate method for date-building?

@jpiasetz

This comment has been minimized.

Contributor

jpiasetz commented May 21, 2013

I pared down the duplication. I'm not sure it worth it to put it in a separate method right now. Might be better to rework the tests first and then come back and refactor it. The matter_hash and writing to file is also a little hairy.

@parkr

View changes

features/step_definitions/jekyll_steps.rb Outdated
else
'%m/%d/%Y' # why even
if "post" == status
if has_time_component?(post['date'])

This comment has been minimized.

@parkr

parkr May 21, 2013

Member

I'd do:

in_format, out_format = if has_time_component?(post['date'])
  ['%Y-%m-%d %H:%M %z'] * 2
else
  ['%m/%d/%Y', '%Y-%m-%d %H:%M']
end
@parkr

View changes

features/step_definitions/jekyll_steps.rb Outdated
post['date'] = parsed_date.strftime(out_format)
folder_post = '_posts'
filename = parsed_date.strftime('%Y-%m-%d') + "-" + filename

This comment has been minimized.

@parkr

parkr May 21, 2013

Member

Whenever I want to interpolate like this, I usually do:

filename = "#{parsed_date.strftime('%Y-%m-%d')}-#{filename}"
@parkr

View changes

jekyll.gemspec Outdated
@@ -37,7 +37,7 @@ Gem::Specification.new do |s|
s.add_development_dependency('rdoc', "~> 3.11")
s.add_development_dependency('redgreen', "~> 1.2")
s.add_development_dependency('shoulda', "~> 3.3.2")
s.add_development_dependency('rr', "~> 1.0")
s.add_development_dependency('rr', "~> 1.0", '!=1.1.0')

This comment has been minimized.

@parkr

parkr May 21, 2013

Member

what's wrong with 1.1.0?

This comment has been minimized.

@jpiasetz

jpiasetz May 21, 2013

Contributor

Didn't look into it in a lot of depth but it was causing my local copy to stop passing and it's what's causing this I think https://travis-ci.org/mojombo/jekyll/jobs/7358372

Probably shouldn't be in this push so I'll remove it.

@mattr-

View changes

features/step_definitions/jekyll_steps.rb Outdated
folder_post = '_drafts'
filename = "#{title}.#{ext}"
elsif "post" == status
in_format, out_format = if has_time_component?(post['date'])

This comment has been minimized.

@mattr-

mattr- May 23, 2013

Member

Could you change this to be something like:

in_format, out_format = time_format(post['date'])

where the time_format method is defined as

def time_format
  return ['%Y-%m-%d %H:%M %z'] * 2 if has_time_component?(post['date'])
  ['%m/%d/%Y', '%Y-%m-%d %H:%M']
end

please?

It's clearer to me what's going on here if I'm just casually reading through the code.

This comment has been minimized.

@jpiasetz

jpiasetz May 23, 2013

Contributor

Done. I extracted a few other methods while I was at it.

@parkr

This comment has been minimized.

Member

parkr commented May 23, 2013

👍 from me!

@mattr-

This comment has been minimized.

Member

mattr- commented May 23, 2013

🤘 Thanks! ❤️

mattr- added a commit that referenced this pull request May 23, 2013

Merge pull request #1124 from jpiasetz/master
Change cucumber tests to not write posts with timezones

@mattr- mattr- merged commit 1cf9efc into jekyll:master May 23, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request May 23, 2013

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.