[Thesoro]

Reference: #13334

This would allow an additional attribute on ng-options to provide a default value for options that lack values.

[Narretz]

Thanks for the PR! Before we can merge we a few things:

• Docs for the new feature
• A test for the new feature
• Squashing all commits
• Update the commit message with the issue that it fixes and with feat instead of fix

Can you work on improving this? If you need help with setting up a dev environment, check https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md or ask here

[Thesoro]

Will do! Thanks for the instructions, I'll add docs and a test, then squash all the commits together.

I'm a little confused about commit messages - I thought I'd included the issue. Should I use "closes" rather than "references"?

Also, the original issue was opened as a bug, has it been reassessed as a feature? (No disagreement if it is! I'm just a newbie to the project and I want to make sure I understand)

[Narretz]

"Closes" in the commit message closes the issue when your commit is merged into master. Which is what we want. :) I say it's a feature, because it's not a bug, it's just something that's missing. Or did this ever work before in some way?

[Thesoro]

That all makes sense to me. I don't believe this has worked before, but I just searched for the "PRs Plz" tag and grabbed one I could make sense of.

[Narretz]

Cool, it's always good to see new contributors. These formal issues (commit message, multiple commits) can be handled when code & docs are ready, they are not blocking.

[Thesoro]

Thanks for your help. When correcting a previously made PR, should I add a comment to that effect?
If so, here is that comment.
If not, uh, never mind.

[Narretz]

Angular expressions are only a subset of Javascript.

[Narretz]

Yes, you should write a commit. Because subscribers don't get notifications for code pushes.

I wonder if the feature is "good enough", because it only works if the option is explicitly selected. I would expect it to work also on load, before any interaction has happened.

[Thesoro]

Okay, I believe I addressed your comments. Thanks for your time in giving those!
I don't really have anything for the pageload problem. I agree that it'd be better if it worked before selection, but that looks like a much trickier fix (at least for someone like me who's still getting his bearings in the Angular source code).

[Narretz]

Good stuff. I have to think about setting the empty option on load. It's probably not necessary to do that.
For the use case described in the original issue, it's not problem, because the model would probably be undefined until it is selected again in the options. The only case I could imagine where this might be useful is when you set your model to a value that is not in your options. In that case, we reset the model to the empty value. So that case is covered.

[Narretz]

You should use the createSingleSelect fn here, to make it consistent with other tests

[Thesoro]

If I use createSingleSelect, how do I give it an ng-empty-value attribute? I was reluctant to change such a heavily used function for just the one test.

[Narretz]

True, didn't think about that. You should be able to use createSelect:

    createSelect({
      'ng-model':'selected',
     'ng-empty-value': 'someVar',
      'ng-options':'value.name for value in values'
    }, true);

[Narretz]

Sorry, I keep finding things. ;)

[Thesoro]

Updated for most of your notes, with the exception of the one I replied about in your line notes. Thanks for your time in looking at these!

[cmplank]

So... is this PR dead? I was trying to use this feature until I realized it was never actually merged. 😞

[gkalpak]

TBH, now that select can be also used with ngRepeat/ngValue, I see little value in adding this (which is only useful in specific usecases).

[Narretz]

I agree that this is not needed in core. Although I don't see how ngValue + select changes the assumptions here - both regular and ngOptions select will set the model to null if the empty option is selected.

In #1780, there are 2 workarounds for this problem, and I've added a 3rd one that implements emptyValue, but doesn't rely on private API.