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 pages by name by default #1848

Merged
merged 3 commits into from
Dec 19, 2013
Merged

Sort pages by name by default #1848

merged 3 commits into from
Dec 19, 2013

Conversation

afeld
Copy link
Contributor

@afeld afeld commented Dec 19, 2013

The order that files are returned differs across operating systems, so ensure that they're being sorted after the fact.

http://stackoverflow.com/a/5529966/358804

The order that files are returned differs across operating systems, so
ensure that they're being sorted after the fact.
@afeld
Copy link
Contributor Author

afeld commented Dec 19, 2013

/cc @benbalter @XhmikosR

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling a6e1c40 on afeld:sort-pages into 12a55b8 on jekyll:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling a6e1c40 on afeld:sort-pages into 12a55b8 on jekyll:master.

@XhmikosR
Copy link
Contributor

Thanks @afeld for this. I totally like this since I was seeing different results on my Windows VM and the GitHub page as you already know :)

@@ -154,6 +154,15 @@ def generate(site)
assert_equal @site.generators.sort_by(&:class).map{|g|g.class.priority}, @site.generators.map{|g|g.class.priority}
end

should "sort pages in a consistent way" do
# The order that files are returned differs across operating systems, so ensure that they're being sorted after the fact.
# http://stackoverflow.com/a/5529966/358804
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information (link and comment) would probably be better for the commit message – would you mind removing them from the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that these lines of code can move around, so the commit information as to why the line is there can be hard to track down. Would moving these comments within the code (vs. leaving them in the test) make it any better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as possible, the only comments should be TomDoc. Can this information not be either encoded in the should string and/or in the Pull Request body? Ideally it wouldn't be needed to understand why the test is there.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 45ee9f8 on afeld:sort-pages into 12a55b8 on jekyll:master.

@afeld
Copy link
Contributor Author

afeld commented Dec 19, 2013

Updated!

@parkr
Copy link
Member

parkr commented Dec 19, 2013

Thanks so much, and sorry to be so negative on comments.

This LGTM. @mattr-?

@mattr-
Copy link
Member

mattr- commented Dec 19, 2013

👍 :shipit:

parkr added a commit that referenced this pull request Dec 19, 2013
@parkr parkr merged commit 94e96d3 into jekyll:master Dec 19, 2013
parkr added a commit that referenced this pull request Dec 19, 2013
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants