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

Add Behance Importer #46

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
4 participants
@rayfranco
Contributor

rayfranco commented Aug 9, 2013

Hello,

— I'm really new to Ruby so, thank in advance for your indulgence —

I've built a little importer draft that works with Behance's API. It imports all projects from a specific user and create new post entries for each project. For now, it's pretty straightforward : all the meta datas returned by the API are stored in the post variable named "project", all of them, as delivered.

So, before going any further, I'd like to have some feedback from you guys.

I'm pretty sure it could be better if the project modules are printed in the post content (instead of relying on the metadatas) and if images are stored locally.

Do this project follow the jekyll-import guidelines? Should I publish it as a standalone import script? And what about new branches for new importers, I haven't seen one. And finally, do you think an importer go against the API terms of use?

Any help and feedback is welcome.

Thanks.

Show outdated Hide outdated lib/jekyll/jekyll-import/behance.rb
require 'date'
require 'time'
require 'behance'

This comment has been minimized.

@parkr

parkr Aug 10, 2013

Member

Please catch the potential LoadError and print a nice message to ask the user to install the gem :)

@parkr

parkr Aug 10, 2013

Member

Please catch the potential LoadError and print a nice message to ask the user to install the gem :)

Show outdated Hide outdated lib/jekyll/jekyll-import/behance.rb
details = client.project(project['id'])
title = project['name'].to_s
formatted_date = Time.at(project['published_on']).to_datetime.to_s

This comment has been minimized.

@parkr

parkr Aug 10, 2013

Member

What happens if we get a botched response and project["published_on"] is nil? Maybe call #to_i on it?

@parkr

parkr Aug 10, 2013

Member

What happens if we get a botched response and project["published_on"] is nil? Maybe call #to_i on it?

Show outdated Hide outdated lib/jekyll/jekyll-import/behance.rb
formatted_date = Time.at(project['published_on']).to_datetime.to_s
post_name = title.split(%r{ |!|/|:|&|-|$|,}).map do |i|
i.downcase if i != ''

This comment has been minimized.

@parkr

parkr Aug 10, 2013

Member

This is a great example of when unless is helpful (also, I changed the variable name to describe what it holds a bit better):

character.downcase unless character.empty?
@parkr

parkr Aug 10, 2013

Member

This is a great example of when unless is helpful (also, I changed the variable name to describe what it holds a bit better):

character.downcase unless character.empty?

This comment has been minimized.

@rayfranco

rayfranco Aug 10, 2013

Contributor

Good to know 👍 Thanks

@rayfranco

rayfranco Aug 10, 2013

Contributor

Good to know 👍 Thanks

Show outdated Hide outdated lib/jekyll/jekyll-import/behance.rb
FileUtils.mkdir_p("_posts")
File.open("_posts/#{name}.html", "w") do |f|

This comment has been minimized.

@parkr

parkr Aug 10, 2013

Member

Any reason you chose HTML over Markdown? Is the content all in HTML?

@parkr

parkr Aug 10, 2013

Member

Any reason you chose HTML over Markdown? Is the content all in HTML?

This comment has been minimized.

@rayfranco

rayfranco Aug 10, 2013

Contributor

This was in prevision of how I would be exporting modules, that have an HTML formatted version as well as a text version. I wasn't even sure if I would like to have the HTML version of the posts. I've switched to Markdown with the project description in the post content.

@rayfranco

rayfranco Aug 10, 2013

Contributor

This was in prevision of how I would be exporting modules, that have an HTML formatted version as well as a text version. I wasn't even sure if I would like to have the HTML version of the posts. I've switched to Markdown with the project description in the post content.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 10, 2013

Member

I'm pretty sure it could be better if the project modules are printed in the post content (instead of relying on the metadatas) and if images are stored locally.

The metadata is nice because it allows more flexibility in styling. If you output modules, it would be a lot harder for me to make my tweaks. I think adding the fields as metadata and maybe a description or something as content, then you're on the right track with Jekyll.

Do this project follow the jekyll-import guidelines? Should I publish it as a standalone import script?

Nah, this is perfect.

And what about new branches for new importers, I haven't seen one.

We merge everything into the main branch here and deploy that to RubyGems.org.

And finally, do you think an importer go against the API terms of use?

As long as it's never anyone else's work (without the consent/attribution of the other owner) then you should be OK.

Member

parkr commented Aug 10, 2013

I'm pretty sure it could be better if the project modules are printed in the post content (instead of relying on the metadatas) and if images are stored locally.

The metadata is nice because it allows more flexibility in styling. If you output modules, it would be a lot harder for me to make my tweaks. I think adding the fields as metadata and maybe a description or something as content, then you're on the right track with Jekyll.

Do this project follow the jekyll-import guidelines? Should I publish it as a standalone import script?

Nah, this is perfect.

And what about new branches for new importers, I haven't seen one.

We merge everything into the main branch here and deploy that to RubyGems.org.

And finally, do you think an importer go against the API terms of use?

As long as it's never anyone else's work (without the consent/attribution of the other owner) then you should be OK.

@rayfranco

This comment has been minimized.

Show comment
Hide comment
@rayfranco

rayfranco Aug 10, 2013

Contributor

@parkr thanks for your encouraging feedback. Here my updated code, raked and tested in a real jekyll environment. Works great, I should maybe make sure all the metadatas are usable, but I guess safe_yaml did its job.

I'am just not sure about importing assets (at least images) in a local folder, but this would go against the API that serve full URL, and this involve rewriting URL in modules, which I think is not a very good idea. What do you think ?

Contributor

rayfranco commented Aug 10, 2013

@parkr thanks for your encouraging feedback. Here my updated code, raked and tested in a real jekyll environment. Works great, I should maybe make sure all the metadatas are usable, but I guess safe_yaml did its job.

I'am just not sure about importing assets (at least images) in a local folder, but this would go against the API that serve full URL, and this involve rewriting URL in modules, which I think is not a very good idea. What do you think ?

Show outdated Hide outdated lib/jekyll/jekyll-import/behance.rb
module JekyllImport
module BehanceImport
def self.validate(options)
if !options[:user]

This comment has been minimized.

@drewish

drewish Aug 11, 2013

Contributor

i'm still pretty new to ruby so take it with a grain of salt, but i feel like it's more ruby-esq to do use unless options[:user] rather than if !options[:user].

@drewish

drewish Aug 11, 2013

Contributor

i'm still pretty new to ruby so take it with a grain of salt, but i feel like it's more ruby-esq to do use unless options[:user] rather than if !options[:user].

This comment has been minimized.

@drewish

drewish Aug 11, 2013

Contributor

and frequently you see it as a modifier:

abort "Missing mandatory option --user." unless options[:user]
@drewish

drewish Aug 11, 2013

Contributor

and frequently you see it as a modifier:

abort "Missing mandatory option --user." unless options[:user]
@rayfranco

This comment has been minimized.

Show comment
Hide comment
@rayfranco

rayfranco Aug 11, 2013

Contributor

@drewish sounds right :) thanks.

Contributor

rayfranco commented Aug 11, 2013

@drewish sounds right :) thanks.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 14, 2013

Member

Looking really great! Any way you could add in some tests for it so we don't break it in the future?

Member

parkr commented Aug 14, 2013

Looking really great! Any way you could add in some tests for it so we don't break it in the future?

@rayfranco

This comment has been minimized.

Show comment
Hide comment
@rayfranco

rayfranco Aug 14, 2013

Contributor

@parkr of course, but I have several questions. Is it a good practice to test the return of an API? If so, how can I test it without providing an API key in the test files?
Also, how can I test jekyll-import output? I can see a helper.rb but I don't see any test using it yet.

Contributor

rayfranco commented Aug 14, 2013

@parkr of course, but I have several questions. Is it a good practice to test the return of an API? If so, how can I test it without providing an API key in the test files?
Also, how can I test jekyll-import output? I can see a helper.rb but I don't see any test using it yet.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 29, 2013

Member

@rayfranco Sorry for the ridiculously long delay! It's not generally good practice to test the API directly, but to test with mock data. That is, you should be able to override your method which accesses the API and send back mock data so you know what to expect.

What do you mean, "test jekyll-import output"?

Member

parkr commented Sep 29, 2013

@rayfranco Sorry for the ridiculously long delay! It's not generally good practice to test the API directly, but to test with mock data. That is, you should be able to override your method which accesses the API and send back mock data so you know what to expect.

What do you mean, "test jekyll-import output"?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 10, 2013

Member

So I've totally reorganized the importers which makes this pull unmergeable as-is.

Member

parkr commented Nov 10, 2013

So I've totally reorganized the importers which makes this pull unmergeable as-is.

@parkr parkr closed this Dec 8, 2013

@parkr parkr referenced this pull request Dec 8, 2013

Merged

Add Behance Importer #104

@rayfranco

This comment has been minimized.

Show comment
Hide comment
@rayfranco

rayfranco Dec 8, 2013

Contributor

Sorry for the late reply, but I currently have no plan to work on this PR since I've moved to a node based generator.

Contributor

rayfranco commented Dec 8, 2013

Sorry for the late reply, but I currently have no plan to work on this PR since I've moved to a node based generator.

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