Skip to content

Add type casting to roleid and resourceid#22

Merged
kgilpin merged 3 commits intomasterfrom
cast
Jan 5, 2014
Merged

Add type casting to roleid and resourceid#22
kgilpin merged 3 commits intomasterfrom
cast

Conversation

@kgilpin
Copy link
Copy Markdown
Contributor

@kgilpin kgilpin commented Dec 23, 2013

No description provided.

@ghost ghost assigned jjmason Dec 23, 2013
Comment thread spec/lib/role_spec.rb Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Huh?

kgilpin and others added 2 commits December 23, 2013 15:56
commit 82d4905b9f61c1131577893e1ecb0215a35e01cc
Author: Jon <jonathan.j.mason@gmail.com>
Date:   Thu Dec 26 08:25:56 2013 -0600

    spec filter casting

commit a9925b841a10ef30b50e041de6724348fad31ef1
Author: Jon <jonathan.j.mason@gmail.com>
Date:   Thu Dec 26 08:20:28 2013 -0600

    cast filter argument
kgilpin added a commit that referenced this pull request Jan 5, 2014
Add type casting to roleid and resourceid
@kgilpin kgilpin merged commit 76eb156 into master Jan 5, 2014
@kgilpin kgilpin deleted the cast branch January 5, 2014 19:50
@dividedmind
Copy link
Copy Markdown
Contributor

Ugh. I know this is old and I should probably shut up, but I don't like this solution - it's so Java-y it hurts ;)
I'd rather add #[to_]roleid to String and Array. Let's use more ruby in ruby! :)
(Would be even better with refinements in 2.0. Though we can't use that here, api being 1.9-compatible.)

@kgilpin
Copy link
Copy Markdown
Contributor Author

kgilpin commented Jan 6, 2014

It's also global namespace pollution...

@dividedmind
Copy link
Copy Markdown
Contributor

Yes, I know, that's why I'm always kind of on the fence about it; another solution would be to be more strict about the types (ie. have a blessed string/array subclass that's explicitly an id). Fortunately the namespace pollution thing is what refinements are meant to solve, so it's going to be less of a painful choice in the future.

conjur-jenkins pushed a commit that referenced this pull request Dec 12, 2025
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.

3 participants