Skip to content

Conversation

ashmaroli
Copy link
Member

Came across a bug that prevented me from accessing /#{API}/data via GIT BASH.

This refactors the implementation for directory support for data files

@ashmaroli
Copy link
Member Author

@fowlie, can you check if this patch causes any test failures at your end?
To pull this to your system, simply run the following within the repo directory:

$ git checkout -b test-branch
$ git pull upstream pull/396/head

@fowlie
Copy link

fowlie commented Jun 14, 2017

@ashmaroli, yes, running script/cibuild on this PR causes these errors (same errors I ended up with in issue #394):

Failures:

  1) collections an individual collection entries lists directories
     Failure/Error: expect(entries.first["name"]).to eq("more posts")
     
       expected: "more posts"
            got: "test"
     
       (compared using ==)
     # ./spec/jekyll-admin/server/collection_spec.rb:94:in `block (4 levels) in <top (required)>'

  2) JekyllAdmin::URLable data files knows the API URL
     Failure/Error: expect(subject.api_url).to eql(api_url)
     
       expected: "http://localhost:4000/_api/data/data_file.yml"
            got: "http://localhost:4000/_api/data/movies/genres/fiction.yml"
     
       (compared using eql?)
     # ./spec/jekyll-admin/urlable_spec.rb:92:in `block (3 levels) in <top (required)>'

Finished in 33.68 seconds (files took 1.47 seconds to load)
176 examples, 2 failures

Failed examples:

rspec ./spec/jekyll-admin/server/collection_spec.rb:90 # collections an individual collection entries lists directories
rspec ./spec/jekyll-admin/urlable_spec.rb:91 # JekyllAdmin::URLable data files knows the API URL

@ashmaroli
Copy link
Member Author

@fowlie Thank you for testing. Sorry I couldn't isolate the cause of failures at your end, but as a consolation, I was able to pick the bug addressed in this PR because I tested in a different environment than my usual dev env. So, thanks once again. 😃

@fowlie
Copy link

fowlie commented Jun 14, 2017

@ashmaroli No problem, happy to help 😄

@mertkahyaoglu
Copy link
Member

I think we should handle this in PathHelper module which contains all path related logic. A few conditional statements would do the trick, I guess.

@ashmaroli
Copy link
Member Author

I think we should handle this in PathHelper module which contains all path related logic. A few conditional statements would do the trick, I guess.

True but since the module is both includedand extended its going to be slightly tricky..

@mertkahyaoglu mertkahyaoglu merged commit 9f9bc99 into jekyll:master Sep 11, 2017
@ashmaroli
Copy link
Member Author

DISCLAIMER: since its been so long, I don't quite remember what my PRs were about.. 😆

@ashmaroli ashmaroli deleted the data-dir-patch branch September 11, 2017 07:25
@mertkahyaoglu
Copy link
Member

Yeah, sorry for that 😬 I could barely find time for several months. But from now on, I believe I can spare more time to Jekyll Admin. It's about time for major changes 🎉

@ashmaroli
Copy link
Member Author

Excited to see what you'll be bringing to the table.. 😃

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants