Feature/macros ono dot one #32

colin-kiegel · 2017-01-14T13:02:51Z

@killercup: Here is the current state of the macros_one_dot_one branch. I continued the work of @badboy from #28.

I added backward compatibility to our existing feature:

allow attributes on fields

I also introduced new features:

select different setter patterns (instead of always mutable), e.g. #[setters(immutable)]
select visibility of setters (public/private), e.g. #[setters(private)]
logging via env_logger for better debugging, e.g. RUST_LOG=derive_builder=trace cargo test

Things I am not yet quite happy with

Parsing of attributes: The attribute representation by the syn crate is already very complex. In addition to that our derive_builder crate has quite a bunch of options which I expect to even grow in the future. This combination leads to quite nested attribute parsing in src/options.rs. I wonder if there is a cleaner approach to it.
I think it's time to add compilation tests or to test the generated tokens (e.g. private setters have a lacking testcase right now)
There are some early but unfinished stubs for getters. I left them in for now, but I suggest to split this into a separate PR (or just implement it). But before I do that split, I would like to get some overall feedback first. :-)

Any suggestions are welcome.

colin-kiegel · 2017-01-14T14:09:50Z

Note: Since all tests run inside of a sub-crate right now, we currently don't have doc-tests anymore. :-/

killercup

Very nice! Looks like the original tests still work :)

I would like to change the attribute names to more explicitly show they are for derive-builder – but I also really want to land this! So I'm okay with not changing them right now.

killercup · 2017-01-14T15:26:40Z

README.md


 And this is how it works:

 ```rust
-#[macro_use] extern crate custom_derive;
+#![feature(proc_macro)]


Don't need this no more!

killercup · 2017-01-14T15:31:56Z

src/options.rs

+}
+
+#[derive(Default, Debug)]
+pub struct OptionsBuilder {


Heh, this code looks like a great indicator for a new "indirect builder" setting!

(Indirect builders are the "original" builder pattern using a struct with the same fields but all are optional.)

killercup · 2017-01-14T15:34:02Z

src/options.rs

+    }
+}
+
+impl OptionsBuilder {


Seriously, the first bunch of methods can be derived, incl. the warn! output.

Also just as a warning, every tooling project I work on at some point ends up dogfooding itself :D

Yes, it felt like irony to write so much builder boilerplate in this crate. LOL. ^^

But how could we bootstrap if derive_builder depends on derive_builder? Shall we depend on the latest previously published version? But if that depends on the version before .. well .. people will notice strange dependency chains in their projects. :-)

Let's just adopt the new rustbuild build system! It can easily download a "stage0" snapshot of derive-builder before building derive-builder stage1, and then build-derive again with the stage1 build to finally get a stage2 version we can work with.

killercup · 2017-01-14T15:35:44Z

src/options.rs

+                _ => {}
+            }
+        }
+        debug!("Ignoring attribute.");


I would not use these attribute names, as they live in a global namespace and "getters" and "setters" is too generic (we just had that in diesel/rocket with "options"). How about something like #[builder_options(setters(pattern = mutable, private = true), getter(public = true)]?

hm, I see what you mean.

I am open to a discussion. However I think we should try to keep it short, since the purpose of this crate is to avoid boiler plate. One of the next logical steps IMO would be to add options per struct field. I.e. prefix, pattern, visibility, etc. could all be changed on a per-field level. At least it should be possible to ignore individual fields - and if we introduce that, we can also introduce overwriting all the other options.

#[derive(Builder)] #[builder_options(setters(pattern = mutable, private = true), getters(public = true)] struct Foo { foo: u32, #[builder_options(setter(prefix = with, pattern = immutable, private = false), getter(prefix = has, public = false)] bar: u32 }

vs.

#[derive(Builder)] #[setters(mutable, private), getters(public)] struct Foo { foo: u32, #[setter(prefix = with, immutable, public), getter(prefix = has, private)] bar: u32 }

Good points, and I think you are right: Let's keep it concise, i.e. call the attributes (s|g)etter(s?).

Another point: Do you think it'd be nicer to have setter and getter options defined in the same , or in two separate attributes?

killercup · 2017-01-14T15:41:58Z

src/options.rs

+            _ => {
+                panic!("Unknown option {:?}", ident)
+            }
+        };


I feel like there should be an idiomatic solution to parsing syn attributes to structs. Maybe another derive crate? A small serde plugin for (de)serializing things to/from rust attributes? The possibilities are infinite! :D

#[derive(Deserialize)] struct Options { setters: SetterOptions, /* … */ } #[derive(Deserialize)] struct GetterOptions { pattern: SetterPattern, public: bool, private: bool } #[derive(Deserialize)] enum SetterPattern { Owned, Mutable, Immutable } // … let attr: syn::Attribute = attr; let opt: Options = parse_attribute!(builder_options);

Yes, this kind of separation is, what I was looking for. :-)
Sounds interesting, I'll have a look at serde. Never used it before, but since everyone is talking about that's probably an education gap I should close anyway. :-)

Yes we definitely need a better approach. I am tracking this in dtolnay/syn#25.

killercup · 2017-01-14T15:42:39Z

src/options.rs

+    }
+
+    fn parse_getters_options(&mut self, nested: &[syn::NestedMetaItem]) -> &mut Self {
+        panic!("TODO: parse getter options -> {:?}", nested);


probably s/panic!/unimplemented!

Actually in published code I would like to get rid of these unimplemented functions. But generally you are right. :-)

killercup · 2017-01-14T16:01:34Z

@colin-kiegel what about the doc tests in lib.rs? They can still do their thing, right?

colin-kiegel · 2017-01-14T17:06:23Z

@killercup It looks like having proc-macro = true in Cargo.toml disables all doc-tests. I think it's a bug: rust-lang/rust#39064

colin-kiegel · 2017-01-14T22:26:03Z

I updated the documentation - we can still change the syntax however.

was fixed by migrating to macros 1.1 this only adds the testcase

killercup · 2017-01-19T11:35:33Z

@colin-kiegel I suggest we merge this and iterate on it in further PRs

colin-kiegel · 2017-01-19T12:13:32Z

ok, done. :-)

Btw: I've already started some refactoring to allow for field-based options. I'll try to wrap it up in a new PR later this evening.

killercup · 2017-01-19T13:01:52Z

Nice, I'm looking forward to that.I'll also try to look into the builder varaints/typed builders again.

…

Am 19.01.2017 um 13:13 schrieb Colin Kiegel ***@***.***>: ok, done. :-) Btw: I've already started some refactoring to allow for field-based options. I'll try to wrap it up in a new PR later this evening. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

badboy and others added 12 commits October 11, 2016 14:57

Implement derive_builder as a procedural macro

1f43bfb

Move tests to proc_macro implementation

4738aa0

Remove mentions of custom_derive

dd39d21

Move tests into subcrate

b9597eb

Run tests on nightly and for test crate only

b5b419a

Mention nightly requirement

d616575

update dependencies

7987e5d

allow attributes on fields

e123cff

cleanup

149dc2d

logging and different setter pattern

7419e31

remove feature gate

47db2a1

refactoring and private setters

1a9c254

colin-kiegel assigned killercup Jan 14, 2017

colin-kiegel requested a review from killercup January 14, 2017 13:03

colin-kiegel added the macros 1.1 label Jan 14, 2017

killercup approved these changes Jan 14, 2017

View reviewed changes

killercup requested a review from badboy January 14, 2017 15:59

dtolnay mentioned this pull request Jan 14, 2017

Simplify processing of structured MetaItems dtolnay/syn#25

Closed

killercup mentioned this pull request Jan 14, 2017

February Meetup: Macros 1.1 Rustaceans/rust-cologne#22

Closed

12 tasks

update documentation

3394c8c

fix #21 missing lifetime support

d15d788

was fixed by migrating to macros 1.1 this only adds the testcase

killercup mentioned this pull request Jan 16, 2017

Typesafe builders #33

Closed

colin-kiegel merged commit 0c1d5ae into master Jan 19, 2017

colin-kiegel deleted the feature/macros-ono-dot-one branch January 19, 2017 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/macros ono dot one #32

Feature/macros ono dot one #32

colin-kiegel commented Jan 14, 2017 •

edited

Loading

colin-kiegel commented Jan 14, 2017

killercup left a comment

killercup Jan 14, 2017

killercup Jan 14, 2017

killercup Jan 14, 2017

colin-kiegel Jan 14, 2017

killercup Jan 14, 2017 •

edited

Loading

killercup Jan 14, 2017

colin-kiegel Jan 14, 2017

killercup Jan 16, 2017

killercup Jan 14, 2017

colin-kiegel Jan 14, 2017

dtolnay Jan 14, 2017

killercup Jan 14, 2017

colin-kiegel Jan 14, 2017

killercup commented Jan 14, 2017

colin-kiegel commented Jan 14, 2017

colin-kiegel commented Jan 14, 2017

killercup commented Jan 19, 2017

colin-kiegel commented Jan 19, 2017

killercup commented Jan 19, 2017 via email

Feature/macros ono dot one #32

Feature/macros ono dot one #32

Conversation

colin-kiegel commented Jan 14, 2017 • edited Loading

colin-kiegel commented Jan 14, 2017

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup commented Jan 14, 2017

colin-kiegel commented Jan 14, 2017

colin-kiegel commented Jan 14, 2017

killercup commented Jan 19, 2017

colin-kiegel commented Jan 19, 2017

killercup commented Jan 19, 2017 via email

colin-kiegel commented Jan 14, 2017 •

edited

Loading

killercup Jan 14, 2017 •

edited

Loading