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

Add derive flag #687

Merged
merged 3 commits into from
Feb 26, 2020
Merged

Add derive flag #687

merged 3 commits into from
Feb 26, 2020

Conversation

vorner
Copy link
Contributor

@vorner vorner commented Feb 18, 2020

  • There are currently two distinct *-derive features and it is
    uncomfortable to hunt for them.
  • They exist mostly due to technical reasons, as they bring two
    different crates. The price of the other if one is already present is
    relatively low (the cost is by depending on syntax and quote and they
    share them).
  • This is in the convention of having the derive feature flag as serde
    and other packages have.
  • The original two are still preserved, so users still can enable only
    one of them.

Closes #684.

Checklist

  • I've added tests for all code changes and additions (where applicable) (not applicable)
  • I've added a demonstration of the new feature to one or more examples
    Should I update the examples to use this feature, or leave them be with the current specs-derive?
  • I've updated the book to reflect my changes
  • Usage of new public items is shown in the API docs

I'm not sure this completely applies. I've updated the note in docs about features, but otherwise left the specs-derive as the main suggestion how to use it by users and expect this will be more like for people who go more by guess than by documentation. But if you prefer, I can update all the examples, etc, to use this new feature flag instead.

API changes

Sorry, something went wrong.

src/lib.rs Outdated
@@ -39,7 +39,7 @@
//! }
//! ```
//!
//! Or alternatively, if you enable the `specs-derive` feature, you can use a
//! Or alternatively, if you enable the `specs-derive` or `derive` feature, you can use a
Copy link
Member

Choose a reason for hiding this comment

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

I think it's alright just mentioning the "derive" feature without mentioning specs-derive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've updated the references through the documentation and tutorials if it's better to just list the one.

I've done a rebase too, since there was a conflict on changelog, but I believe with a diff this small the added difficulty to review is basically none.

vorner and others added 3 commits February 25, 2020 18:29
* There are currently two distinct *-derive features and it is
  uncomfortable to hunt for them.
* They exist mostly due to technical reasons, as they bring two
  different crates. The price of the other if one is already present is
  relatively low (the cost is by depending on syntax and quote and they
  share them).
* This is in the convention of having the `derive` feature flag as serde
  and other packages have.
* The original two are still preserved, so users still can enable only
  one of them.

Closes #684.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@azriel91 azriel91 left a comment

Choose a reason for hiding this comment

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

Cool thanks!

@azriel91
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 26, 2020

Build succeeded

@bors bors bot merged commit b33baa9 into amethyst:master Feb 26, 2020
@vorner vorner deleted the derive-feature branch February 26, 2020 07:12
@vorner
Copy link
Contributor Author

vorner commented Feb 26, 2020

Thank you :-)

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.

All-inclusive derive feature
2 participants