-
-
Notifications
You must be signed in to change notification settings - Fork 731
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 #122 #123
base: main
Are you sure you want to change the base?
issue #122 #123
Conversation
the removal of tests shows that this would be a breaking change. i need to think about this. |
@MadManMathew If you're still interested in landing this, could you rebase (not merge) on top of latest master? |
@ljharb yes will do |
@ljharb I messed up the rebase, will fix. |
no worries, just ping me when you'd like me to take a look! |
@MadManMathew if you're still interested in landing this, please use my version of your tests (from 75fe7ce) and let's get it working without removing any existing tests. Changing them slightly may end up being fine. |
@MadManMathew if you're not interested in completing this PR, would you mind checking the "allow edits" checkbox on the right hand column? |
@ljharb sorry will do |
I've rebased this and included my tests; but I'm not sure how to adapt your fix to make the tests pass. |
@MadManMathew are you interested in pursuing this PR? |
#122
let me know your'e thoughts, I know remove test cases isn't ideal but I removed the functionality where we make the value the key and the value of that key true.