refactoring: re-export custom derive with additional items (WIP) #94

colin-kiegel · 2017-04-26T22:23:25Z

This allows to add other item definitions as described in https://users.rust-lang.org/t/10546/2

renamed derive_builder -> derive_builder_macro
added new derive_builder, which re-exports derive_builder_macro
moved all testcases into new crate testsuite
moved no_std testcase into a separate crate inside testsuite.
This way we have better control, e.g.
- panic="abort" compiler flag
- only run on nightly
- compile derive_builder with no_std feature

WIP

The no_std test is still failing. Somehow derive_builder still re-exports std. This is probably due to the glob-export in derive_builder/src/lib.rs:530
```
pub use derive_builder_macro::*;
```
But it would seem very difficult to write derive_builder_macro without std. There must be a different way - especially since serde seems to be doing something like this successfully. Could this be the trick?

this allows to add other item definitions as described in https://users.rust-lang.org/t/10546/2

move `no_std` testcase into a separate crate inside `testsuite`. This way we have better control, e.g. - panic="abort" compiler flag - only run on nightly - compile `derive_builder` with no_std feature

somehow `cd testsuite && cargo test --all` pulled in tests from `no_std`, despite the fact that it was (a) not in the workspace and (b) only an optional dependency. Now we disable the test via a feature flag `nightlytests`.

TedDriggs · 2017-04-27T01:52:17Z

I'd request we merge #92 before embarking on this, or else I'll have a hell of a time merging that later. I can look at this next week and see if I can figure out what's causing problems.

colin-kiegel · 2017-04-27T08:28:52Z

Done. I also updated this branch. :-)

It was surprisingly simple - git was able to figure everything out - except of the location of a new file.

git checkout reexport_macro_with_items
git merge master
git mv derive_builder/tests/bounds_generation.rs testsuite/tests/
git commit --amend

Also removed duplication of no_std test.

TedDriggs · 2017-04-29T00:18:07Z

I think I've got most of it figured out now; dev/githook.sh passes cleanly, which is promising. I've added the pub mod export, and as a result gutted most of the Bindings struct (which is now an enum with Std and NoStd).

colin-kiegel · 2017-04-29T08:39:25Z

@TedDriggs Ok, cool. I merged your changes into from your fork into this branch.

dev/nightlytests.sh seems to be still failing (at least for me) - I get the same error as before

error[E0152]: duplicate lang item found: `eh_personality`.
  --> no_std/tests/no_std.rs:51:1
   |
51 | pub extern  fn eh_personality() {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: first defined in crate `panic_unwind`.


error[E0152]: duplicate lang item found: `panic_fmt`.
  --> no_std/tests/no_std.rs:61:1
   |
61 | / pub extern fn rust_begin_panic(_msg: core::fmt::Arguments,
62 | |                                _file: &'static str,
63 | |                                _line: u32) -> ! {
64 | |     unsafe { intrinsics::abort() }
65 | | }
   | |_^
   |
   = note: first defined in crate `std`.

error: aborting due to 2 previous errors

Note: You should also be able to push directly to the branch in this repository. If your local branch is also called reexport_macro_with_items, then you should be able to do something like:

git remote add upstream git@github.com:colin-kiegel/rust-derive-builder.git
git push --set-upstream upstream reexport_macro_with_items

colin-kiegel

I left some inline comments

colin-kiegel · 2017-04-29T08:49:08Z

derive_builder/src/lib.rs

+    pub use ::core::default::Default;
+    pub use ::core::marker::PhantomData;
+    pub use ::core::option::Option::{self, Some, None};
+    pub use ::core::result::Result::{self, Ok, Err};


I wonder if this can lead to problems if crate A exports FooBuilder and crate B wants to use that builder. Without depending on derive_builder, all those re-exports would not be in scope of crate B.

We should double check if that is a problem.

(Serde seems to do somethings similar https://github.com/serde-rs/serde/blob/6fbf40b83c65ba2bfe9e94276482b13dd979ac63/serde/src/export.rs)

It doesn't seem to; it looks like the system is smart enough to know that these are actually the same ones as in ::std, or in ::core

colin-kiegel · 2017-04-29T08:52:20Z

derive_builder_macro/src/options/field_mode.rs

-            // Attribute { style: Outer, value: NameValue(Ident("doc"), Str("/// This is a doc comment for a field", Cooked)), is_sugared_doc: true }
+    // example:
+    // Attribute { style: Outer, value: NameValue(Ident("doc"), Str("/// This is a
+    // doc comment for a field", Cooked)), is_sugared_doc: true }


the indentation should not change here

colin-kiegel · 2017-04-29T08:52:25Z

derive_builder_macro/src/options/field_mode.rs

-            // Attribute { style: Outer, value: List(Ident("allow"), [MetaItem(Word(Ident("non_snake_case")))]), is_sugared_doc: false }
+    // example:
+    // Attribute { style: Outer, value: List(Ident("allow"),
+    // [MetaItem(Word(Ident("non_snake_case")))]), is_sugared_doc: false }


the indentation should not change here

colin-kiegel · 2017-04-29T08:55:00Z

testsuite/tests/no_std

@@ -1 +0,0 @@
-../no_std/tests/


didn't you like this symlink?

I thought it would make it easier to find the no_std testcases.

It was causing build problems for me for some reason.

TedDriggs · 2018-05-16T15:18:35Z

@colin-kiegel as a heads-up, I'm going to take another stab at this. Since we don't support generating multiple builders off a single struct, it still makes sense to me to expose a Builder trait with an associated type for the kind of builder returned. This would give us a means to expose a structured "Result" type.

One major change though: Somehow the crate name builder is still available. Rather than massively disrupt this crate, I'm going to make a lightweight builder crate that in turn depends on derive_builder for now.

Edit: Having just written this, I'm waffling on the trait argument. Callers would have to import the type they wanted to build and the trait (blegh), which seems less ergonomic than importing two types from the crate they're directly consuming.

colin-kiegel added 3 commits April 26, 2017 23:25

reexport custom derive

6ff20d6

this allows to add other item definitions as described in https://users.rust-lang.org/t/10546/2

refactor no_std testcase (WIP)

ce03426

move `no_std` testcase into a separate crate inside `testsuite`. This way we have better control, e.g. - panic="abort" compiler flag - only run on nightly - compile `derive_builder` with no_std feature

fix no_std on stable

195c445

somehow `cd testsuite && cargo test --all` pulled in tests from `no_std`, despite the fact that it was (a) not in the workspace and (b) only an optional dependency. Now we disable the test via a feature flag `nightlytests`.

This was referenced Apr 26, 2017

structured errors instead of String #60

Closed

Reexport macro with items TedDriggs/rust-derive-builder#1

Open

fix travis discrepancy with devtools

ebb17b7

colin-kiegel mentioned this pull request Apr 26, 2017

Refactoring Ideas #95

Closed

7 tasks

Merge branch 'master' into reexport_macro_with_items

d30f416

TedDriggs added 4 commits April 28, 2017 09:18

Switched to export types for everything but String

9b298fe

Remove most of derive_builder_core::Bindings

07cf253

Continue to clean up bindings

6c383c4

Added TryInto back to Bindings until trait stabilizes

7102a32

Also removed duplication of no_std test.

colin-kiegel commented Apr 29, 2017

View reviewed changes

TedDriggs closed this May 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactoring: re-export custom derive with additional items (WIP) #94

refactoring: re-export custom derive with additional items (WIP) #94

colin-kiegel commented Apr 26, 2017 •

edited

Loading

TedDriggs commented Apr 27, 2017

colin-kiegel commented Apr 27, 2017

TedDriggs commented Apr 29, 2017

colin-kiegel commented Apr 29, 2017

colin-kiegel left a comment

colin-kiegel Apr 29, 2017

TedDriggs May 30, 2017

colin-kiegel Apr 29, 2017

colin-kiegel Apr 29, 2017

colin-kiegel Apr 29, 2017

TedDriggs May 30, 2017

TedDriggs commented May 16, 2018 •

edited

Loading

		@@ -1 +0,0 @@
		../no_std/tests/

refactoring: re-export custom derive with additional items (WIP) #94

refactoring: re-export custom derive with additional items (WIP) #94

Conversation

colin-kiegel commented Apr 26, 2017 • edited Loading

TedDriggs commented Apr 27, 2017

colin-kiegel commented Apr 27, 2017

TedDriggs commented Apr 29, 2017

colin-kiegel commented Apr 29, 2017

colin-kiegel left a comment

Choose a reason for hiding this comment

colin-kiegel Apr 29, 2017

Choose a reason for hiding this comment

TedDriggs May 30, 2017

Choose a reason for hiding this comment

colin-kiegel Apr 29, 2017

Choose a reason for hiding this comment

colin-kiegel Apr 29, 2017

Choose a reason for hiding this comment

colin-kiegel Apr 29, 2017

Choose a reason for hiding this comment

TedDriggs May 30, 2017

Choose a reason for hiding this comment

TedDriggs commented May 16, 2018 • edited Loading

colin-kiegel commented Apr 26, 2017 •

edited

Loading

TedDriggs commented May 16, 2018 •

edited

Loading