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

Basic image retrieval, cmdargs does not work #134

Merged
merged 2 commits into from May 23, 2014

Conversation

Projects
None yet
3 participants
@engina
Contributor

engina commented Apr 18, 2014

This patch:

  1. Retrieves the image resources in the blog post to assets folder
  2. Modifies the blog post so that it uses the downloaded resources

Bugs:
Command line arguments does not work.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 18, 2014

Member

The command-line args will work once 2.0 drops, but sadly it's been mucked up because Jekyll 1.x uses Commander which uses symbol keys, but Jekyll 2.x uses Mercenary which uses string keys so it's a whole thing.

This looks pretty good! It'd be baller if you could separate this stuff out into a separate method so we can unit test it, then... you guessed it: add a quick test for it! Thank you!

Member

parkr commented Apr 18, 2014

The command-line args will work once 2.0 drops, but sadly it's been mucked up because Jekyll 1.x uses Commander which uses symbol keys, but Jekyll 2.x uses Mercenary which uses string keys so it's a whole thing.

This looks pretty good! It'd be baller if you could separate this stuff out into a separate method so we can unit test it, then... you guessed it: add a quick test for it! Thank you!

@engina

This comment has been minimized.

Show comment
Hide comment
@engina

engina Apr 18, 2014

Contributor

I did my best, though review this patch with grain of salt as I do not know Ruby at all (Yes, I've lost my Ruby virginity for this patch).

Note: I forgot to mention that this patch also REPLACES any class in PRE elements to "prettyprint".

These settings worked the best for me but side affects must be considered.

Contributor

engina commented Apr 18, 2014

I did my best, though review this patch with grain of salt as I do not know Ruby at all (Yes, I've lost my Ruby virginity for this patch).

Note: I forgot to mention that this patch also REPLACES any class in PRE elements to "prettyprint".

These settings worked the best for me but side affects must be considered.

end
(content/"pre").each do |pre|
pre["class"] = 'prettyprint'

This comment has been minimized.

@parkr

parkr Apr 19, 2014

Member

Why do this?

@parkr

parkr Apr 19, 2014

Member

Why do this?

@engina

This comment has been minimized.

Show comment
Hide comment
@engina

engina Apr 19, 2014

Contributor

To pretty print code snippets (duh!).

This may probably better be handled in some other stage -- i.e. when generating the actual HTML pages. Feel free to remove it if there's a better way to handle syntax highlighting.

Contributor

engina commented Apr 19, 2014

To pretty print code snippets (duh!).

This may probably better be handled in some other stage -- i.e. when generating the actual HTML pages. Feel free to remove it if there's a better way to handle syntax highlighting.

@parkr parkr self-assigned this Apr 21, 2014

parkr added a commit that referenced this pull request May 23, 2014

@parkr parkr merged commit ac8615e into jekyll:master May 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

parkr added a commit that referenced this pull request May 23, 2014

parkr added a commit that referenced this pull request Apr 14, 2016

@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.