fix(ng:list) : ng-list directive separator bug fix. #1841
Conversation
|
Firefox 18.0 (Linux) input ngList should parse text into an array FAILED This test case is failing since original code was appending space after hard coded comma separator which I corrected by specifying only the given separator. Is space supposed to be append by $formatters function ? |
|
I think this is a case for changing the spec. But you still need to add On 19 January 2013 12:15, ashish [email protected] wrote:
|
|
Hi , I have also resolved this issue by applying new patch but it caused some May I go ahead and reapply the patch if Travis is stabilized ? Thanks, On Sat, Jan 19, 2013 at 8:04 PM, Pete Bacon Darwin <[email protected]
|
|
The build is still giving failure after re-applying patch . |
|
Personally I think that this needs some more thought. I would expect that converting to a list and back again should be idempotent. So to hack in a space seems dubious. It would seem to me that one should either live without spaces being magically added, in which case the current spec is wrong and needs changing. Or that there should be some option to get the directive to add spaces when serializing if that is what you want. In either case you still need to add new unit test specs for the new functionality that you have added - i.e. that the separator will be used in serializing rather than |
|
By the way the Travis build failing has nothing to do with your code. If you look the unit tests are all passing. |
|
In fact there is one more tricky issue here: |
|
@petebacondarwin , you mean to say I need to add test specs along with commits for test coverage ? What about the test cases getting failed due to original implementation ? |
|
Yes, and if the original test spec is incorrect it should be changed. On 22 January 2013 06:00, ashish [email protected] wrote:
|
|
@pkozlowski-opensource, I would like to summarize my proposed implementation which would take care of reg-expression also. Case-1) If ng-list is an reg-expr then we can take an extra attribute for separator (default to comma) so that model to view binding can work consistently. I think leaving it up-to user what he wants to see is a better idea here. Case-2) If ng-list is not reg-expr then simply we use original delimeter provided in ng-list. Let me know your opinion about it !!! @petebacondarwin , can you give some suggestions ? |
|
Need to see a failing test case. All of the tests pass on or CI server, so I am not sure why you think we need this change. |
|
Hi Misko, Unfortunately test-cases for ng-list do not cover the case to ensure whether model->view binding is ok or not with ng-list as a reg-expresson, hence it passed all test-cases. That is what I am trying to convey in my previous comments. Let me know you views on it. |
|
|
@ashishgithub Your PR needs to contain a test that should be failing without your fix. This way we can now that your commit fixes an issue demonstrated by a test. This PR will require a bit of additional work before it can be considered for merging, I've included a checklist for you. But you should really start with a test. |
|
There hasn't been any work on this PR recently. I'm closing it to keep the PR queue clean. Please re-open the PR when you continue work on it. Thanks! |
|
Are you actually closing a PR that fixes an obvious and easily verifiable bug? Shouldn't at least the issue remain open? If having a clean PR queue is more important than actually fixing bugs, I have to say I'm a bit worried about the priorities of the project. |
|
I've created a new pull request that fixes this issue: #2561 |
Issue : ng-list separator getting overridden by ',' .