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

Add included_keys filter (tests broken atm) #140

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

h0jeZvgoxFepBQ2C
Copy link
Contributor

@h0jeZvgoxFepBQ2C h0jeZvgoxFepBQ2C commented Sep 17, 2019

Adds a filter to only include specified keys (see #139).

If ignored_keys is specified too, it will search for keys which are included, but are not ignored.

Tests are not running through, but I don't understand why exactly.
It seems that Lit.included_keys is not reset during test runs, even when the loader is set to nil?

Do you maybe have an idea @mlitwiniuk ?

@mlitwiniuk
Copy link
Member

Hi @h0jeZvgoxFepBQ2C , I'm back online, I'll take a look at this PR later this week.

@mlitwiniuk
Copy link
Member

@h0jeZvgoxFepBQ2C could you update PR with rebased master? .travis.yml got changed and now CI should run properly

@h0jeZvgoxFepBQ2C
Copy link
Contributor Author

Rebased it again, would be cool if you could take a look, thanks!

@mlitwiniuk mlitwiniuk self-assigned this May 31, 2020
@h0jeZvgoxFepBQ2C
Copy link
Contributor Author

I've rebased the PR, would be great if someone could take a look at it.

Copy link
Member

@mlitwiniuk mlitwiniuk left a comment

Choose a reason for hiding this comment

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

@h0jeZvgoxFepBQ2C I think the only remark I have is how variable is called. What do you think?

@@ -10,6 +10,7 @@ module Lit
mattr_accessor :humanize_key
mattr_accessor :humanize_key_ignored_keys
mattr_accessor :humanize_key_ignored
mattr_accessor :included_keys
Copy link
Member

Choose a reason for hiding this comment

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

I think that this variable name is misleading. How about only_accepted_keys ?

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.

2 participants