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

Pull request for feature #259 #260

Closed
wants to merge 4 commits into from
Closed

Pull request for feature #259 #260

wants to merge 4 commits into from

Conversation

Nataliya
Copy link
Contributor

@Nataliya Nataliya commented Dec 2, 2011

Pull request for feature #259

@rsutphin
Copy link
Contributor

rsutphin commented Dec 3, 2011

I do not think this design is particularly well considered.

  • The syntax for embedding values looks like part of a ruby hash embedded in a string, but isn't. Example from the features:
label "Thank you for using :prepopulate=>[test_global_config.center]"

The use of rubyish syntax implies that something like this would work:

label "Thank you for using :prepopulate => [test_global_config.center]"

But it won't.

  • Similarly, the :prepopulate=> part of the template is unnecessary. It will be the same for every templated text, so making it a word (instead of just some sort of marker like just the => or {{) makes the whole thing harder to read without any benefits.

  • This solution only allows for filtering in values loaded from YAML files in a hard-coded path. This will not work even in the application where we need to use this feature first (where these sort of "global" configuration values come from a file that's outside the application, isn't a YAML file, and already has its own separate parsing logic in order to provide for validation and default values). It also does not easily allow for sharing the survey definition with Surveyor iOS. It's definitely not a good design for a generic solution.

    In general, why treat application-static filtered values as different from any others? A Surveyor-using app should be able to provide a set of values to be filtered in (possibly as part of the ResponseSet) from whatever sources it thinks are appropriate. Surveyor shouldn't care where they come from.

    Recall also that we would eventually like to be able to filter in values from the ResponseSet as it is being filled out. While it's reasonable to limit the scope for the first pass at the feature to values which exist outside of the template, you would want the syntaxes for filtering in values from all sources to be the same. I don't see how this syntax would work with internal values.

  • This solution implements its own template parser. There are lots and lots of existing template languages for ruby. There's no reason why we should be implementing our own. I'd suggest using either ERB (since it's built into the language and so widely familiar) or Mustache (since it has a very clean, readable, language-agnostic syntax). Mustache has implementations in Ruby, JavaScript, and Objective-C, so it could potentially be used both on the client side and in Surveyor iOS.

@Nataliya Nataliya closed this Dec 7, 2011
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.

2 participants