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 from support for the setters #21

Merged
merged 1 commit into from
May 27, 2019
Merged

Add from support for the setters #21

merged 1 commit into from
May 27, 2019

Conversation

roblabla
Copy link
Contributor

This PR adds the ability for setters to take a type to convert back into an u32.

u32, from into FooBar, field4, set_field4: 10, 0;

will generate the following setter:

fn set_field4(&mut self, value: FooBar) {
    use bitfield::BitRange;
    self.set_bit_range(10, 0, bitfield::Into::<u32>::into(value));
}

This allows making more uniform APIs to access fields.

@dzamlo
Copy link
Owner

dzamlo commented May 23, 2019

Thanks a lot for your pull request!

Maybe we should make the setter generic and accept any T: From (or the type of the field), even if we don't specify a keyword. This way all setter will be more flexible. What do think? Do you see any drawbacks to that?

Also, I don't see any tests. I will have time on Monday to add them if you don't do that in the meantime.

@roblabla
Copy link
Contributor Author

Maybe we should make the setter generic and accept any T: From (or the type of the field), even if we don't specify a keyword.

I don't think that's better. From a documentation perspective, I want the setter to take a specific type so the user can easily access that type and get more documentation about it. As a practical example, here I want to ensure the user will only ever handle instances of DeliveryMode. Its representation as an u32 should be an implementation detail.

And yeah, I didn't write any tests (apart from the doctest I guess). I'll make up some time tomorrow to write them :).

@dzamlo dzamlo merged commit c5901de into dzamlo:master May 27, 2019
@dzamlo
Copy link
Owner

dzamlo commented May 27, 2019

Released in 0.13.2

@dzamlo
Copy link
Owner

dzamlo commented May 27, 2019

I thought of something else. It would be a breaking change, but wouldn't it make sense to set the setter type when there is just the into keyword? Making the from useless.

  • no into: both the getter and setter get the normal type
  • into: both the getter and setter get the into type
  • if you want different type for the getter and setter, use two field, with _ as the setter for one and _ as the getter for the other.

The where you want into with a setter that use another seems pretty rare. And you can still achieve that with two field.

@roblabla
Copy link
Contributor Author

Yeah, I didn't want to make a breaking change but having into alone affect the setter seems ideal. If this is going to be tackled, it'd be nice to fix #7 at the same time since it would also require a breaking change.

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