-
Notifications
You must be signed in to change notification settings - Fork 2
Add a mock_all_slots option to create_thing_without_server
#199
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
Conversation
386cc5b to
48b1aaa
Compare
|
Great :) Glad this works, and it seems like it wasn't too much code to implement it. I am thinking that it is probably a sensible idea to create a submodule called |
48b1aaa to
2709890
Compare
Barecheck - Code coverage reportTotal: 94.66%Your code coverage diff: 0.12% ▴ Uncovered files and lines
|
|
I understand I think moving it to a testing module is probably a good idea. I like the idea of:
I think it makes it very clear that this not intended for actual use. |
rwb27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. I've made a couple of suggestions that might be nice, but don't change the way it works meaningfully.
It would be good to move this to a testing module as we discussed, I'm happy for you to do that now, or we can do it later.
|
Yeah this is only targeted to #195 so it has a sensible diff. I think aiming it at main once those are merged makes sense |
0cf53bb to
ed2e66f
Compare
9d7e399 to
4a65419
Compare
3ecec6e to
143884d
Compare
9562f17 to
f76d2d3
Compare
rwb27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great - thanks for all the rebasing and updates to test code.
I spotted one thing, which is that you defined a constant for a Thing name but then didn't use it. That's trivial enough that I'll make the change and merge it now - no need to bother you with it.
Adds a
mock_all_slotsoption tocreate_thing_without_server. This will follow the default specified bything_slot, so if the default isNoneno mock is created.If it is a mapping with no default
I thinkone instance is created, thisneeds checking in a testis tested.I have targeted it at #195 so the diff is clear.now targets mainCloses #198