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

Integrate RON #269

Merged
merged 6 commits into from Aug 6, 2017

Conversation

@torkleyy
Copy link
Member

commented Aug 4, 2017

No description provided.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

looks good to me, but for some reason the documentation is failing to generate?

@kvark
kvark approved these changes Aug 4, 2017
Copy link
Member

left a comment

This is nuts:

  • ~400 LOC removed
  • cleaner actual configuration files
  • less macro magic on the Rust side

/me 😍 it!

#[serde(default)]
pub multisampling: u16,
/// Sets the visibility of the window.
#[serde(default = "true")]

This comment has been minimized.

Copy link
@kvark

kvark Aug 4, 2017

Member

I think it doesn't like this "true". Perhaps, quoting should not be here?

This comment has been minimized.

Copy link
@torkleyy

torkleyy Aug 4, 2017

Author Member

Yeah, I know that it doesn't work yet. For whatever reason it accepts a function name. I'll try to make a PR for #[serde(default_value=...)].

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

@torkleyy https://serde.rs/attr-default.html Woops, it doesn't seem to like specifying default literals like that.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

@torkleyy While we wait for the default_value suggestion to land, how do you feel about the commit I made just now?

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

And now we have another problem :/

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

Ah. serde_derive doesn't work without serde.

@vitvakatu

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2017

@Xaeroxe stable travis passed, neat

@torkleyy

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2017

Yeah, we can use it for now.

@torkleyy

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2017

But examples won't work right now.

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

@torkleyy mind if I fix them?

@Xaeroxe Xaeroxe force-pushed the torkleyy:ron branch from 863ddd3 to 607a6f1 Aug 4, 2017

@Xaeroxe

This comment has been minimized.

Copy link
Member

commented Aug 4, 2017

@torkleyy torkleyy#1 Merge if you please

@Xaeroxe
Xaeroxe approved these changes Aug 4, 2017
Copy link
Member

left a comment

Works great! Ready to merge as far as I'm concerned.

@Xaeroxe Xaeroxe requested a review from Aug 4, 2017

@Xaeroxe Xaeroxe force-pushed the torkleyy:ron branch from 0d6fe8e to 5b91144 Aug 4, 2017

@Xaeroxe Xaeroxe merged commit a662404 into amethyst:develop Aug 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@torkleyy torkleyy deleted the torkleyy:ron branch Aug 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.