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

Sort the results of the require_all glob. #4912

Merged
merged 2 commits into from
May 19, 2016
Merged

Conversation

parkr
Copy link
Member

@parkr parkr commented May 19, 2016

Filesystems behave differently when performing glob listings.

In my environment, they are listed alphabetically. On my Mac, when asking for a list of files in a directory, those files are returned as a nicely sorted list. Alphabetized, like you'd want them to be. Like you'd expect them to be.

In some environments, quite different from my own, the return of a similar operation is quite random. Perhaps q comes before a, or e before d; the filesystem will choose its order of the day and you, the fare user, tired and weary from work, must bare the brunt of this.

And so, with this commit, I do hereby request that the noble makers of Dir[] provide for us, the downtrodden and ravaged users, some consistency. As a user of Ruby, I shouldn't have to know or consider the behaviour of an individual filesystem here; it should function the same for all filesystems.

Truly yours,
Parker.

Filesystems behave differently when performing glob listings.

In my environment, they are listed alphabetically. On my Mac, when asking for a list of files in a directory, those files are returned as a nicely sorted list. Alphabetized, like you'd want them to be. Like you'd expect them to be.

In some environments, quite different from my own, the return of a similar operation is quite random. Perhaps q comes before a, or e before d; the filesystem will choose its order of the day and you, the fare user, tired and weary from work, must bare the brunt of this.

And so, with this commit, I do hereby request that the noble makers of Dir[] provide for us, the downtrodden and ravaged users, some consistency. As a user of Ruby, I shouldn't have to know or consider the behaviour of an individual filesystem here; it should function the same for all filesystems.

Truly yours,
Parker
@parkr parkr added this to the 3.1.5 milestone May 19, 2016
@parkr
Copy link
Member Author

parkr commented May 19, 2016

Error I am attempting to fix only occurs on certain Linux machines (not travis?):

NameError: uninitialized constant Jekyll::Drops::DocumentDrop
  .bundle/ruby/2.1.0/gems/jekyll-3.1.4/lib/jekyll/drops/excerpt_drop.rb:5:in `<module:Drops>'
  .bundle/ruby/2.1.0/gems/jekyll-3.1.4/lib/jekyll/drops/excerpt_drop.rb:4:in `<module:Jekyll>'
  .bundle/ruby/2.1.0/gems/jekyll-3.1.4/lib/jekyll/drops/excerpt_drop.rb:3:in `<top (required)>'
  .bundle/ruby/2.1.0/gems/jekyll-3.1.4/lib/jekyll.rb:11:in `require'
  .bundle/ruby/2.1.0/gems/jekyll-3.1.4/lib/jekyll.rb:11:in `block in require_all'
  .bundle/ruby/2.1.0/gems/jekyll-3.1.4/lib/jekyll.rb:10:in `each'
  .bundle/ruby/2.1.0/gems/jekyll-3.1.4/lib/jekyll.rb:10:in `require_all'
  .bundle/ruby/2.1.0/gems/jekyll-3.1.4/lib/jekyll.rb:174:in `<top (required)>'

@envygeeks
Copy link
Contributor

More importantly why do you need it to be sorted? If you need it sorted sounds to me like somebody somewhere is doing something wrong.

@parkr
Copy link
Member Author

parkr commented May 19, 2016

@envygeeks I pasted the error just seconds before you posted your comment; you may have missed it. Sorry I didn't include it in the original post!

@envygeeks
Copy link
Contributor

@parkr IMO we should just drop that magic and start doing require_relative through and through?

@envygeeks
Copy link
Contributor

What I mean is jekyll.rb loads everything with require_relative inside of jekyll/ and jekyll/drops.rb loads everything inside of jekyll/drops/, that will also solve people not wanting to load the entirety of Jekyll to get a single piece.

@parkr
Copy link
Member Author

parkr commented May 19, 2016

@envygeeks The funny thing is this has worked for ages... It's only just caused problems today. Why don't we just fix this as-is now and move to a better solution in v3.2.

What I mean is jekyll.rb loads everything with require_relative inside of jekyll/ and jekyll/drops.rb loads everything inside of jekyll/drops/, that will also solve people not wanting to load the entirety of Jekyll to get a single piece.

@envygeeks You mean every file requires its dependencies? That'll be a bit difficult to test, as we could easily miss dependencies... I'm a huge fan of the autoload route which loads things as needed.

@envygeeks
Copy link
Contributor

Why don't we just fix this as-is now and move to a better solution in v3.2.

It's those kind of problems/push-offs that lead to what I'm about to say below... sadly.

You mean every file requires its dependencies? That'll be a bit difficult to test, as we could easily miss dependencies... I'm a huge fan of the autoload route which loads things as needed.

What do you mean? If you forget to have a dependency load... the tests for that dependency should fail and if they don't that's a problem in and of itself. It should fail the same way that currently does. If you fear that happening then I think all development need be stalled until the tests are brought up to snuff.

@parkr
Copy link
Member Author

parkr commented May 19, 2016

It's those kind of problems/push-offs that lead to what I'm about to say below... sadly.

This wasn't a problem before and I just want to get a 3.1.5 out the door tonight with a fix for the above NameError. Feel free to submit a PR to take this PR's place if you want.

If you forget to have a dependency load... the tests for that dependency should fail

I think we're talking past each other here – I mean that in our unit tests, these won't necessarily fail because one file could load, say Jekyll::Utils into the runtime, and cause us to miss that another file requires Jekyll::Utils but doesn't have the require_relative "./utils" or whatever. Either that, or our tests need to be remade so each file that's being tested is loaded by itself – currently we just load all of Jekyll so we'd miss if a file doesn't load in the right dependencies because we already loaded everything.

@envygeeks
Copy link
Contributor

I agree!

@parkr parkr merged commit cafbd83 into 3.1-stable May 19, 2016
@parkr parkr deleted the 3.1-sort-includes branch May 19, 2016 04:19
@jekyll jekyll locked and limited conversation to collaborators Jul 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants