Skip to content
This repository has been archived by the owner on May 15, 2023. It is now read-only.

change description maxLen truncation toggle #114

Closed
wants to merge 1 commit into from
Closed

Conversation

noahdietz
Copy link
Contributor

  • passing maxLen: -1 toggles off all test description truncation, the length helper becomes a noop
  • updated README to reflect this option
  • changes .eslintrc to allow longer line length in source files

Refers to discussion in #107

@noahdietz
Copy link
Contributor Author

@Remco75 please review when you get a chance. It is quite small.

@mm-gmbd I noticed in your PR that you had the Handlebars helpers in another folder. I like the idea, I think I did that on my own fork too but never merged it...configurable variables will have to be exported via helpers.js and set by index.js. Just a heads up that you'll have to add it to your PR. 😄

@Remco75
Copy link
Contributor

Remco75 commented Sep 5, 2016

Nice job, lgtm! One remark, would be nice to only truncate if the user explicitly asks for it. So, in absence of the minLength config you get the full description (makes a bit more sense to me, but I don't know about historical reasons).
So, if you want to keep it this way: merge it

On a completely unrelated note: while writing tests I noticed we have a lot of code duplication (for instance, to load the compare code).
Might be handy to write a helper function for it. But that's for a new PR

@mm-gmbd
Copy link
Contributor

mm-gmbd commented Sep 5, 2016

@noahdietz - I moved the helpers to their own file based on what I saw in your fork :)

@noahdietz
Copy link
Contributor Author

@Remco75 I would refer you to the conversation we had on #107 prior to your arrival in the data-generation conversation. Its a matter of generating valid javascript consistently. I'd rather a user be "upset" that their description was truncated and turn off truncation then have to go through each test file and change stuff. In any other situation, i'd agree, making functionality that mutates user info off by default is convention.

@mm-gmbd perfect! Think I should make a PR on your fork to include it in your PR rather than making you redo this work to merge yours? I can just copy + paste most of the stuff, and add the variable export to helpers.js....it'll be pretty simple.

@Remco75
Copy link
Contributor

Remco75 commented Sep 5, 2016

@noahdietz in that case, merge it!

@noahdietz
Copy link
Contributor Author

@mm-gmbd I copied this into a PR for your repo, but I don't have permission to make a new repo! so if you don't mind doing the work to merge your PR and after this one, then I will merge this. Otherwise, if you could give me some permissions on your repo, I'd be happy to submit the change there so you don't have to!

@noahdietz
Copy link
Contributor Author

This will be closed when #4 from mm-gmd's fork is merged and #107 is merged to master

@Remco75 Remco75 mentioned this pull request Sep 26, 2016
@noahdietz noahdietz closed this Oct 13, 2016
@noahdietz noahdietz deleted the length-config branch May 5, 2017 21:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants