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

Add roller importer #363

Merged
merged 12 commits into from Oct 17, 2018

Conversation

Projects
None yet
5 participants
@pmb00cs
Copy link
Contributor

commented Oct 14, 2018

I've written an importer for apache roller, it is based on the wordpress importer but modified for the apache roller database structure. It appears to work for my apache roller blog.

@DirtyF DirtyF requested a review from jekyll/plugin-core Oct 14, 2018

@ashmaroli

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

@pmb00cs Numerous style offenses detected in your proposal.
Please run bundle exec rubocop --auto-correct locally and push the changes.
Thanks.

pmb00cs added some commits Oct 15, 2018

@pmb00cs

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

I've run the rubocop job with auto correct, and also fixed the fact that it didn't import comments.

@DirtyF DirtyF added the feature label Oct 15, 2018

@ashmaroli

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@pmb00cs What this needs now is some tests.
Don't forget to run git pull origin add_roller_importer to update your local branch with the commits I pushed.. 😉

@pmb00cs

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Happy to write some tests, what sort of tests do you want?

I've created a basic test file based off the wordpress test file (as I initially based my importer off the wordpress importer) but this seems sort of sparse.

@ashmaroli

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

what sort of tests do you want?

Awesome! ❤️ your attitude!! 👍
At minimum, the test should cover Importers::Roller.process(opts). That one method covers pretty much everything the Importer is supposed to do..

@pmb00cs

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

I'll try and write some tests for that soon.

@parkr

parkr approved these changes Oct 16, 2018

Copy link
Member

left a comment

We don't have a lot of tests for importers, and it's not super destructive for people so I'd be fine merging and adding tests later if it came to that!

@ashmaroli

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@parkr Can we have at least one test covering the "query string" here..?
Honestly, I don't know how that string is ultimately used and how one can refactor it without any consequences..

FROM weblogentry AS `weblogentry`
LEFT JOIN roller_user AS `roller_user`
ON weblogentry.creator = roller_user.username"
SELECT

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Oct 17, 2018

Member

@pmb00cs May I ask why this string had to be indented by an additional 4 spaces to overcome the frozen string literal issue..?

This comment has been minimized.

Copy link
@pmb00cs

pmb00cs Oct 17, 2018

Author Contributor

You can ask, but I'm afraid the answer is probably not very satisfying.

This is the string I was having trouble being frozen, and in my naivety, I assumed the issue lay somewhere around here, so I made many changes to this string. I had thought I had put everything back as it was, but clearly not. After my changes and running bundle exec rubocop --auto-correct again this is what the result is. I'd love to blame rubocop, but it is far more likely my ad-hoc corrections were to blame.

Simply put it didn't, and this got included in my commit because of how I was editing the file. The fix is the two changes of << to += further down.

This string was changed with the addition of three lines to get the extra information I used to set the permalink.

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Oct 17, 2018

Member

Thank you for the response @pmb00cs
I now see that you had added the following to lines to the "query string" as well, which was not immediately evident from the diff.

  LEFT JOIN weblog AS `weblog`
    ON weblogentry.websiteid = weblog.id

There is a bit of an inconsistency regarding the "query strings" in the class overall, though..
Here, the string starts with a \n..

  posts_query = "
        SELECT
          weblogentry.id           AS `id`,
          ..."

elsewhere there is this:

  cquery =
    "SELECT
       weblogcategory.name AS `name`

Unfortunately, since I don't know how these strings are ultimately handled (no tests!), I won't be able to suggest a refactoring..

@ashmaroli

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@pmb00cs You can open another PR to add tests for this when ready.. I won't press you on adding tests right away..

@ashmaroli ashmaroli removed the needs-tests label Oct 17, 2018

@DirtyF

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit d7540d7 into jekyll:master Oct 17, 2018

2 checks passed

WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Oct 17, 2018

@DirtyF

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@pmb00cs Let me know if it's OK to release a new minor version of this gem with current implementation. 💎

@pmb00cs

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

So far I believe it does everything I needed of it. If you're happy with it I'd be very happy to have my contribution included in your project.

@ashmaroli

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@pmb00cs Your contribution has already been added to the project.
You're officially a Contributor 🎉
Thank you very much!!

@DirtyF

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.