Introduce Response#value, an abstraction over Surveyor's value storage. #310

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@yipdw
Contributor

yipdw commented Apr 25, 2012

This commit also makes some other simplifications:

  1. The value key in the JSON representation of a response should always
    be present, as a response always has a value. Previously, this was
    not the case.
  2. Answer#response_class is a string, not a symbol. ActsAsResponse#as
    expects symbols, but is usually invoked with a response class as its
    first argument. This commit resolves the string/symbol inconsistency
    for these methods and eliminates some unnecessary interning.
    (There's a #to_s in the case condition for backwards
    compatibility; at some point, that ought to be removed.)
  3. "return case" in ActsAsResponse#as is redundant.

NB: there's no Response#value= here because the corresponding writer, IMO, only makes sense in NCS Core or other applications setting values from JSON representations of Responses. If, however, that functionality is desired in Surveyor, I can add it to this pull request.

David Yip
Introduce Response#value, an abstraction over Surveyor's value storage.
This commit also makes some other simplifications:

1. The value key in the JSON representation of a response should always
   be present, as a response always has a value.  Previously, this was
   not the case.

2. Answer#response_class is a string, not a symbol.  ActsAsResponse#as
   expects symbols, but is usually invoked with a response class as its
   first argument.  This commit resolves the string/symbol inconsistency
   for these methods and eliminates some unnecessary interning.
   (There's a #to_s in the case condition for backwards
   compatibility; at some point, that ought to be removed.)

3. "return case" in ActsAsResponse#as is redundant.
@yoon

This comment has been minimized.

Show comment Hide comment
@yoon

yoon May 4, 2012

Member
  1. A response doesn't always have a value - only if Answer#response_class is string, text, integer, float, date, datetime, or time.
  2. In the DSL Question#pick and Answer#response_class are allowed to be symbols for ease of writing. We should make the behavior of the two consistent - either overridding the pick= as is done in the Question class, or another solution in the parser.
Member

yoon commented May 4, 2012

  1. A response doesn't always have a value - only if Answer#response_class is string, text, integer, float, date, datetime, or time.
  2. In the DSL Question#pick and Answer#response_class are allowed to be symbols for ease of writing. We should make the behavior of the two consistent - either overridding the pick= as is done in the Question class, or another solution in the parser.
@yipdw

This comment has been minimized.

Show comment Hide comment
@yipdw

yipdw May 4, 2012

Contributor
  1. A Response still has a value; it's just nil. (And it's fine if it's nil for answers -- the proposed Response#value returns nil in that case.)
  2. I agree that the #to_s should not exist. However, I still strongly recommend string usage:
  • response_class is stored as a string;
  • to the best of my knowledge, no major Ruby implementation now or in the near future will subject unused symbols to garbage collection; and
  • the number of possibilities for Answer#response_class are unbounded.

IMO, if the sole justification for symbols is to save a character, then the parser should take care of converting those symbols to strings.

Contributor

yipdw commented May 4, 2012

  1. A Response still has a value; it's just nil. (And it's fine if it's nil for answers -- the proposed Response#value returns nil in that case.)
  2. I agree that the #to_s should not exist. However, I still strongly recommend string usage:
  • response_class is stored as a string;
  • to the best of my knowledge, no major Ruby implementation now or in the near future will subject unused symbols to garbage collection; and
  • the number of possibilities for Answer#response_class are unbounded.

IMO, if the sole justification for symbols is to save a character, then the parser should take care of converting those symbols to strings.

@jdzak

This comment has been minimized.

Show comment Hide comment
@jdzak

jdzak Aug 14, 2012

Member

Having this would make it much easier to get a response's value. Putting it in surveyor's codebase makes sense because it's probably something most people have written in their application to get a response value. The code looks fine.

+1 to pull

Member

jdzak commented Aug 14, 2012

Having this would make it much easier to get a response's value. Putting it in surveyor's codebase makes sense because it's probably something most people have written in their application to get a response value. The code looks fine.

+1 to pull

@yoon yoon closed this Feb 6, 2013

@yoon

This comment has been minimized.

Show comment Hide comment
@yoon

yoon Feb 6, 2013

Member

we jave built consensus in #311 around having Response#value replace the other _value attributes

Member

yoon commented Feb 6, 2013

we jave built consensus in #311 around having Response#value replace the other _value attributes

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