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.

Sign up for GitHub

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

Jump to bottom

stats update - none stable sort #1123

Merged
merged 1 commit into from
Nov 18, 2017
Merged

stats update - none stable sort #1123

merged 1 commit into from
Nov 18, 2017

Conversation

MSP-Greg
Copy link
Contributor

@MSP-Greg MSP-Greg commented Oct 7, 2017

Description

Recent Ruby builds may introduce unstable sorting; this was causing test failures with Windows builds.

Ran fork with Travis.

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@coveralls
Copy link

coveralls commented Oct 7, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 93.508% when pulling 328e2b4 on MSP-Greg:stats into c546a8a on lsegal:master.

@lsegal
Copy link
Owner

lsegal commented Oct 7, 2017

Thanks for the PR. I have a few questions:

  1. Can you point to which versions are affected by this?
  2. Is Ruby moving away from stable sort or is this just a bug in those builds?
  3. Is there a bug tracker link you can point to with more info?

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Oct 7, 2017 •
edited
Loading

  1. Can you point to which versions are affected by this?

2.4 and trunk have the following text in array#sort - 'The result is not guaranteed to be stable.' See ruby commit 5b3b855 svn 56413.

There were two PR's/commits in RubyGems for this issue. Sometimes I've seen with_index used, other times the array/multiple sort keys style. I asuumed you'd be okay with path as a good 'force unique' key.

@lsegal lsegal mentioned this pull request Nov 17, 2017
Closed
@lsegal
Copy link
Owner

lsegal commented Nov 18, 2017

@MSP-Greg wondering if you had a chance to look at these PR comments?

@MSP-Greg
Copy link
Contributor Author

@lsegal

Sorry, I didn't see the reference from two days ago. What does ' It looks like it's blocked on PR updates'
refer to?

@lsegal
Copy link
Owner

lsegal commented Nov 18, 2017

@MSP-Greg I've made comments on the PR above that need addressing before this can be merged

@MSP-Greg
Copy link
Contributor Author

I thought the edits to my 2nd Oct 7 comment addressed those, If nobu adds to the docs about it, I think that's pretty final...

@lsegal
Copy link
Owner

lsegal commented Nov 18, 2017

Oh damn, it looks like I didn't finalize my PR. I'm sorry!

lsegal
lsegal requested changes Nov 18, 2017
View reviewed changes
Copy link
Owner

@lsegal lsegal left a comment

Choose a reason for hiding this comment

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

Minor nitpicks here

lib/yard/cli/stats.rb Outdated

objects = @undoc_list.sort_by {|o| o.file.to_s }
max = objects.sort_by {|o| o.path.length }.last.path.length
# array needed for sort due to none stable sorting
Copy link
Owner

Choose a reason for hiding this comment

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

  • Add newline before this comment
  • "none stable sorting" should read "unstable sort"

@coveralls
Copy link

coveralls commented Nov 18, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 93.499% when pulling c373be6 on MSP-Greg:stats into dab964c on lsegal:master.

@lsegal lsegal merged commit a3d4d1f into lsegal:master Nov 18, 2017
@lsegal
Copy link
Owner

lsegal commented Nov 18, 2017

Thanks for fixing, sorry about the PR confusion!

@MSP-Greg
Copy link
Contributor Author

Well, I'm the one that used the interesting phrase 'none stable sort'.

Let me know if you'd like to add Appveyor. Also, Travis could be updated to 2.2.8, 2.3.5, and 2.4.2.

Thanks.

@MSP-Greg MSP-Greg deleted the stats branch November 18, 2017 21:16
lsegal added a commit that referenced this pull request Nov 18, 2017
@lsegal
Tag release v0.9.10
9fcfe09 
References:
  #1023, #1077, #1079, #1089, #1093, #1097, #1099, #1101,
  #1102, #1104, #1111, #1119, #1123, #1124, #1126, #1128,
  #1133, #1135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsegal lsegal lsegal requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MSP-Greg @coveralls @lsegal