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

Lower default lines of UD response to 5, adjust corresponding spec #2

Merged
merged 2 commits into from Jan 30, 2015

Conversation

chriswoodrich
Copy link
Contributor

@chriswoodrich
Copy link
Contributor Author

This fixes issue #1

@chriswoodrich
Copy link
Contributor Author

@jimmycuadra pls

@jimmycuadra
Copy link
Owner

I think 20 is a reasonable default, but it'd be great to expose this limit as a configuration option if people want to change it.

I also noticed that the definition that brought on this discussion was resulting in about 45 lines of response, so this PR will not address that problem – there's some other bug happening that's making it bypass this hard coded limit of 20.

@chriswoodrich
Copy link
Contributor Author

I'll add a config ceiling and investigate the bug at the same time.

In the mean time, can you merge this to thwart @tristaneuan ?

@jimmycuadra
Copy link
Owner

It won't have any effect as is, due to the bug I mentioned. :\

if lines.size > 20
lines = lines[0..19]
# strip out tabs and spaces baked into lines repsonse
lines = lines.map {|line| line.split(/[\r\n]+/)}.flatten
Copy link
Owner

Choose a reason for hiding this comment

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

Let's move this logic into its own method so the comment isn't needed. Use flat_map instead of map + flatten for improved performance. For style, put a space after { and before }.

@jimmycuadra
Copy link
Owner

Looking pretty good! The new option should be added to the README, as well.

@chriswoodrich
Copy link
Contributor Author

Will do, @jimmycuadra

@jimmycuadra
Copy link
Owner

I just pushed up a commit that fixes the bug where long examples don't get truncated and adds better test coverage. That will simplify this branch. Here's what's left:

  • Rebase onto the latest master.
  • Remove format_lines and the call to it, since it's no longer needed.
  • Don't set a default value for max_response_size. As is, if someone didn't want responses truncated, the only way to do it would be to fake it by setting the response size to a really high number, which isn't a great user interface.
  • Make sure the logic handles the case where max_response_size is nil. (Add a test for this case.)
  • Update the docs to reflect the lack of default value.
  • Squash your commits down to one for a clean history.

Thanks! :D

@chriswoodrich
Copy link
Contributor Author

@jimmycuadra
Copy link
Owner

lolz, my bad

@chriswoodrich
Copy link
Contributor Author

This is better UI. Default max response is now 20 lines, hard-coding is removed giving the user more control, and a nil value of the max_response_size will not truncate responses at all. I added tests for nil and for a user-set ceiling of 10.

@@ -43,9 +49,19 @@ def fetch_definition(term)
end
end

def determine_max_size(lines)
unless config.max_response_size.nil?

Choose a reason for hiding this comment

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

y u do dis? Change to if/else/end

@jimmycuadra
Copy link
Owner

Looks great. I agree with Mitch though, unless/else is a pattern to avoid. Switch that up and then I'll merge it. Thanks!!

@jimmycuadra
Copy link
Owner

Actually, I can't merge this cleanly yet. You'll have to rebase your own commits onto the latest master. git fetch upstream && git rebase upstream/master

@chriswoodrich
Copy link
Contributor Author

Ready to go, @jimmycuadra

jimmycuadra added a commit that referenced this pull request Jan 30, 2015
Lower default lines of UD response to 5, adjust corresponding spec
@jimmycuadra jimmycuadra merged commit f797025 into jimmycuadra:master Jan 30, 2015
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.

None yet

3 participants