Skip to content

keyword function return default in error cases #995

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

Merged
merged 2 commits into from
Aug 24, 2024

Conversation

mitch-kyle
Copy link
Contributor

@mitch-kyle mitch-kyle commented Aug 24, 2024

Fixes #997

Quick fix. I noticed that using a keyword as a function with nil as the collection always returns nil rather than the default.

This demonstrates the bug:

(:foo nil :bar)

;; Expected 
=> :bar

;;Actual
=> nil

@mitch-kyle mitch-kyle changed the title keyword function return default when not found keyword function return default in error cases Aug 24, 2024
@mitch-kyle
Copy link
Contributor Author

Looks like the failure was with storing the test result rather than the test itself.

Copy link
Member

@chrisrink10 chrisrink10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea Clojure just let any value pass here. Yuck. But thanks for catching this. Two very minor comments on the changelog.

CHANGELOG.md Outdated
@@ -17,9 +17,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Improved on the nREPL server exception messages by matching that of the REPL user friendly format (#968)
* Types created via `deftype` and `reify` may declare supertypes as abstract (taking precedence over true `abc.ABC` types) and specify their member list using `^:abstract-members` metadata (#942)
* Load functions (`load`, `load-file`, `load-reader`, etc) now return the value of the last form evaluated. (#984)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you undo this removal?

CHANGELOG.md Outdated
### Fixed
* Fixed inconsistent behavior with `basilisp.core/with` when the `body` contains more than one form (#981)
* Fixed using keyword as a function not returning the default value in some cases (#995)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you file a ticket w/ the issue and use that as the reference here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@chrisrink10
Copy link
Member

Looks like the failure was with storing the test result rather than the test itself.

These seem to be transient. I reran it and it was fine.

@mitch-kyle
Copy link
Contributor Author

I had no idea Clojure just let any value pass here. Yuck.

In scheme, nil is the empty list. This behaviour is carried over to Clojure. If you do (assoc nil :a 1) you get {:a 1}; it's the same line of thinking.

@mitch-kyle
Copy link
Contributor Author

Comments have been addressed, I'm not what's happening with the readthedocs build and it doesn't look like I have the power to retry it

@chrisrink10
Copy link
Member

I had no idea Clojure just let any value pass here. Yuck.

In scheme, nil is the empty list. This behaviour is carried over to Clojure. If you do (assoc nil :a 1) you get {:a 1}; it's the same line of thinking.

I am not referring to nil. Your change allows keywords to respond to any value and simply ignore it. Clojure seems to allow the same:

user=> (:kw 3 :default)
:default
user=> (:kw "hi" :default)
:default
user=> (:kw "hi")
nil

@chrisrink10
Copy link
Member

Comments have been addressed, I'm not what's happening with the readthedocs build and it doesn't look like I have the power to retry it

Looks like another transient failure. I reran and it was fine.

@chrisrink10 chrisrink10 merged commit d0b46c9 into basilisp-lang:main Aug 24, 2024
12 checks passed
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.

Keyword as function ignoring default value in exceptional cases.
2 participants