Skip to content

refactor Target serialization #2219

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

Merged
merged 4 commits into from
Dec 17, 2015
Merged

Conversation

matklad
Copy link
Member

@matklad matklad commented Dec 16, 2015

@alexcrichton another preparation PR for #2196

I've removed obscure metadata field from Target. It is a breaking change (for read-manifest), but the field seemed cryptic, useless and untested :)

Target has a bunch of boolean fields:

    tested: bool,
    benched: bool,
    doc: bool,
    doctest: bool,
    harness: bool, // whether to use the test harness (--test)
    for_host: bool,

I guess they should not be included in serialized representation?

I will push commits for other Encodables here.

Remove obscure `metadata` field and implement proper Encodable for
TargetKind becase default one is not used.
@rust-highfive
Copy link

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@matklad
Copy link
Member Author

matklad commented Dec 16, 2015

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned huonw Dec 16, 2015
@matklad
Copy link
Member Author

matklad commented Dec 16, 2015

I'm not sure what fields of dependency should I serialize:

pub struct DependencyInner {
    name: String, // already included
    source_id: SourceId, // should be included, but is `source_id` a good name? Maybe just `source`?
    req: VersionReq, // already included
    specified_req: Option<String>, // no sure about this one
    kind: Kind, // should be included?
    only_match_name: bool, // should be omitted

    optional: bool, // should be included
    default_features: bool, // should be omitted
    features: Vec<String>, // should be omitted

    // This dependency should be used only for this platform.
    // `None` means *all platforms*.
    only_for_platform: Option<String>, // should be omitted
}

Otherwise I'm ready to merge this

use support::{project, execs, main_file, basic_bin_manifest};
use hamcrest::{assert_that};

fn setup() {}

fn remove_all_whitespace(s: &str) -> String {
let re = Regex::new(r"\s+").unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a regex for this, could this use .split_whitespace().collect()?

@alexcrichton
Copy link
Member

I think it's ok to leave out the other Target fields for now, they generally meant for somewhat niche configurations that Cargo itself is likely the only one to care about.

For dependency information, here's what I think should happen:

pub struct DependencyInner {
    name: String, // include
    source_id: SourceId, // include (naming 'source' is fine)
    req: VersionReq, // include
    specified_req: Option<String>, // omit
    kind: Kind, // include
    only_match_name: bool, // omit

    optional: bool, // include
    default_features: bool, // include
    features: Vec<String>, // include

    only_for_platform: Option<String>, // include, probably call 'target'
}
``

@matklad
Copy link
Member Author

matklad commented Dec 17, 2015

only_for_platform: Option, // include, probably call 'target'

Should I make a custom encodable for this? Or "target": null, is clear enough. The custom encodable could do "target": "all_platforms",

@matklad
Copy link
Member Author

matklad commented Dec 17, 2015

Ok, I think this is done. It has about 0 tests, but I hope to improve this situation in the metadata PR =)

fn encode<S: Encoder>(&self, s: &mut S) -> Result<(), S::Error> {
match *self {
Kind::Normal => "normal",
Kind::Development => "development",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may want to just be called "dev" as that's what it's called in the manifest

@alexcrichton
Copy link
Member

target encoding as null sounds good to me, just a few last nits and should be good to go!

@matklad
Copy link
Member Author

matklad commented Dec 17, 2015

@alexcrichton updated

@alexcrichton
Copy link
Member

@bors: r+ 77e7273

@bors
Copy link
Contributor

bors commented Dec 17, 2015

⌛ Testing commit 77e7273 with merge 3775b3f...

bors added a commit that referenced this pull request Dec 17, 2015
@alexcrichton another preparation PR for #2196

I've removed obscure `metadata` field from `Target`. It is a breaking change (for read-manifest), but the field seemed cryptic, useless and untested :)

`Target` has a bunch of boolean fields:

```
    tested: bool,
    benched: bool,
    doc: bool,
    doctest: bool,
    harness: bool, // whether to use the test harness (--test)
    for_host: bool,
```

I guess they should not be included in serialized representation?

I will push commits for other `Encodable`s here.
@bors
Copy link
Contributor

bors commented Dec 17, 2015

@bors bors merged commit 77e7273 into rust-lang:master Dec 17, 2015
@matklad matklad deleted the encodable-audit branch June 9, 2016 10:56
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.

5 participants