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

Adding specs for the has_key_for? method #1287

Merged
merged 3 commits into from
Oct 23, 2020
Merged

Conversation

jwoertink
Copy link
Member

@jwoertink jwoertink commented Oct 23, 2020

Purpose

This is to accompany luckyframework/avram#500

Description

This adds in specs for the new params.has_key_for? method. Though the method is defined in Avram, we can't accurately write specs for it in Avram since a user's params will actually be Lucky::Params. In this case, it's closer to "real world" tests.

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

@jwoertink jwoertink changed the title Adding new has_key_for? method to params. Fixes Avram #72 Adding specs for the has_key_for? method Oct 23, 2020
@jwoertink jwoertink merged commit cbd05b9 into master Oct 23, 2020
@jwoertink jwoertink deleted the features/params_has_key_for branch October 23, 2020 18:19
@paulcsmith
Copy link
Member

IMO I think we should remove this from Lucky and keep it in Avram. Right now we could change has_key_for? to anything we want in Avram and we wouldn't know if it works or is broken until we pull master into Lucky and run the specs here. I think since the logic is in Avram the spec should be there. I left an example here: luckyframework/avram#500 (comment) of how it could be done

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