zakarumych / egui-snarl

Customizable egui node-graph library
https://crates.io/crates/egui-snarl
MIT License
252 stars 25 forks source link

Refactoring. Breaking change! #5

Closed nrot closed 8 months ago

nrot commented 8 months ago

What done:

Questions about background pattern api:

zakarumych commented 8 months ago

I'll review it later, but to answer your questions:

SnarlStyle::default method can't be const because const trait impls are not stable yet.

Background style is in Option because None means not "no pattern", it means "unspecified". Trait object for pattern belongs to enum variant Other

nrot commented 8 months ago

I mean SnarStyle::new don't have any argument, why just not use Trait Default::default without const ? For what you need use const for new ? Background::None also mean "no pattern" ?

I'am discussion with you. If you say - i just step back and take for other things

zakarumych commented 8 months ago

Having fn new() with no arguments is pretty common. And implementing Default::default via call to it to ensure they are consistent. After that adding const to pure functions is just a habit of mine. This way you won't have a problem if you suddenly need use it in constant context. But I don't add const to functions that may become non-pure in future without signature change, since removing const would be a breaking change.

nrot commented 8 months ago

Ok, i will implementing Default::default. What about BackgroundPattern::None ? This more simple than use Option<BackgroundPattern>

zakarumych commented 8 months ago

SnarlStyle already implements Default. BackgroundPattern::None means "no pattern", bg_pattern: None means unspecified, and there' can be some default pattern. For no-pattern you may add BackgroundPattern::NoPattern

nrot commented 8 months ago

Please review when you can

zakarumych commented 8 months ago

This looks good to me!

Thank you, I will merge it. I have some other changes to push before release though.