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

Issue 418 #423

Merged
merged 6 commits into from
Feb 14, 2016
Merged

Issue 418 #423

merged 6 commits into from
Feb 14, 2016

Conversation

kbknapp
Copy link
Member

@kbknapp kbknapp commented Feb 9, 2016

No description provided.

@yo-bot
Copy link

yo-bot commented Feb 9, 2016

r? @sru

(yo-bot has picked a reviewer for you, use r? to override)

@sru
Copy link
Contributor

sru commented Feb 9, 2016

@kbknapp Having is_present to return true when default value is set feels a bit weird IMO, mainly because the argument is actually not present. Otherwise, looks good.

@kbknapp
Copy link
Member Author

kbknapp commented Feb 9, 2016

That's a good point that I hadn't thought of....let me see if I find a way
to NOT set that (or occurrences of).

On Wed, Feb 10, 2016, 1:21 AM Sung Rim Huh [email protected] wrote:

@kbknapp https://github.com/kbknapp Having is_present to return true
when default value is set feels a bit weird IMO, mainly because the
argument is actually not present. Otherwise, looks good.


Reply to this email directly or view it on GitHub
#423 (comment).

@kbknapp
Copy link
Member Author

kbknapp commented Feb 10, 2016

So now occurrences_of will show 0 but is_present will still return true because it would take some massive changes to not return true. This is because is_present only checks if an argument exists (which happens when there's a value) it has no way of knowing if the value is a default or not.

I think with the docs being very specific about default values and how that interacts with things like is_present or occurrences_of this will be a good compromise.

@kbknapp kbknapp closed this Feb 10, 2016
@kbknapp kbknapp reopened this Feb 10, 2016
@kbknapp
Copy link
Member Author

kbknapp commented Feb 10, 2016

Didn't mean to close....

@kbknapp
Copy link
Member Author

kbknapp commented Feb 14, 2016

@homu r+

@homu
Copy link
Contributor

homu commented Feb 14, 2016

📌 Commit 33fe291 has been approved by kbknapp

homu added a commit that referenced this pull request Feb 14, 2016
@homu
Copy link
Contributor

homu commented Feb 14, 2016

⌛ Testing commit 33fe291 with merge 6b4d387...

homu added a commit that referenced this pull request Feb 14, 2016
@homu
Copy link
Contributor

homu commented Feb 14, 2016

💔 Test failed - status

@kbknapp
Copy link
Member Author

kbknapp commented Feb 14, 2016

@homu r+

@homu
Copy link
Contributor

homu commented Feb 14, 2016

📌 Commit b37c010 has been approved by kbknapp

homu added a commit that referenced this pull request Feb 14, 2016
@homu
Copy link
Contributor

homu commented Feb 14, 2016

⌛ Testing commit b37c010 with merge e08fdfb...

homu added a commit that referenced this pull request Feb 14, 2016
@homu
Copy link
Contributor

homu commented Feb 14, 2016

☀️ Test successful - status

@homu homu merged commit b37c010 into master Feb 14, 2016
@kbknapp kbknapp deleted the issue-418 branch March 11, 2016 16:17
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.

4 participants