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

Defaults and enable features on rust playground #155

Closed
wants to merge 8 commits into from

Conversation

dns2utf8
Copy link
Contributor

@dns2utf8 dns2utf8 commented Feb 24, 2018

I tested this change with all features enabled and each one independently enabled.
Current state:

  • all enabled
  • independently
  • docs

I use this to test this PR:

extern crate uuid;

use uuid::Uuid;
use uuid::UuidVersion::*;

fn main() {
    let v = vec![ ("Mac", Mac), ("Dce", Dce), ("Md5", Md5), ("Random", Random), ("Sha1", Sha1) ];
    for variant in v {
        let my_uuid = Uuid::new(variant.1);
        println!("{:?} -> {:?}", variant.0, my_uuid);
    }
}

@KodrAus
Copy link
Member

KodrAus commented Feb 25, 2018

Thanks @dns2utf8! Do you have a case where you need a random v3 or v5 uuid? My understanding was that those types are typically used to get a consistent uuid value for some other identifier. The thing here I'm not sure of is defaulting to the NAMESPACE_DNS root, but maybe the default we'd use for random v3 and v5 uuids is a bit arbitrary either way.

Cargo.toml Outdated
@@ -1,7 +1,7 @@
[package]

name = "uuid"
version = "0.6.0"
version = "0.6.1"
Copy link
Member

Choose a reason for hiding this comment

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

We don't touch version number until preparing a release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dns2utf8
Copy link
Contributor Author

hi @KodrAus
When I read the docs I was under the impression the feature could be useful, so I don't have an immediate usecase apart from the learning effect on how features in rust work.

I stumbled on uuid after playing on the playground and I would like the playground to include all the features.

@dns2utf8 dns2utf8 changed the title Defaults Defaults and enable features on rust playground Feb 25, 2018
@Dylan-DPC-zz
Copy link
Member

Hi. Thanks for the contribution but I still don't see the need for this. It just feels like we are adding more complexity for no gain.

@dns2utf8
Copy link
Contributor Author

One pro would be the crate will work as expected on https://play.rust-lang.org/?gist=118112c0ca3014d3683dfd6db7fa99ef&version=stable

@KodrAus
Copy link
Member

KodrAus commented Feb 25, 2018

Ah I see! Yeh there's not a lot you can do without some of the version features enabled. Making uuid more useful on the playground sounds good to me, even without defaulting more uuid versions.

Is the playground feature just a conventional name that play.integer32.com picks up and includes?

@Dylan-DPC-zz Dylan-DPC-zz added Pending:Changes Requested breaking This causes a breaking change and will be merged to breaking branch. labels Feb 26, 2018
@Dylan-DPC-zz Dylan-DPC-zz added this to the 0.7.0 milestone Feb 26, 2018
@shepmaster
Copy link

Is the playground feature just a conventional name that play.integer32.com picks up and includes?

Yep, but both playgrounds have the same content.

kinggoesgaming
kinggoesgaming previously approved these changes Feb 26, 2018
@kinggoesgaming kinggoesgaming dismissed their stale review February 26, 2018 16:28

changes requested were applied but overall approval still needs to go for

@KodrAus
Copy link
Member

KodrAus commented Feb 26, 2018

My thoughts are that we should go ahead with just the playground feature here, and then talk about values for other Uuid::new types in another PR.

What do you all think?

@kinggoesgaming
Copy link
Member

👍 from me

@Dylan-DPC-zz
Copy link
Member

Yeah @KodrAus I agree with that

@Dylan-DPC-zz Dylan-DPC-zz changed the base branch from master to breaking March 13, 2018 13:05
@Dylan-DPC-zz Dylan-DPC-zz changed the base branch from breaking to master March 14, 2018 10:47
@Dylan-DPC-zz Dylan-DPC-zz removed breaking This causes a breaking change and will be merged to breaking branch. Pending:Master Unfreeze labels Mar 14, 2018
@Dylan-DPC-zz
Copy link
Member

@dns2utf8 can you update your branch with master?

@Dylan-DPC-zz Dylan-DPC-zz modified the milestones: 0.7.0, 0.6.2 Mar 14, 2018
@kinggoesgaming kinggoesgaming modified the milestones: 0.6.2, 0.6.3 Mar 21, 2018
@kinggoesgaming
Copy link
Member

@dns2utf8 can you merge master into defaults branch and fix the conglicts

@kinggoesgaming
Copy link
Member

@dns2utf8 I want this to be available in uuid@0.6.3 can you merge changes from master and fix the conflicting changes?

@kinggoesgaming kinggoesgaming removed this from the 0.6.3 milestone Mar 29, 2018
@kinggoesgaming kinggoesgaming mentioned this pull request Mar 30, 2018
4 tasks
bors bot added a commit that referenced this pull request Mar 30, 2018
189: Allow playground defaults r=Dylan-DPC a=kinggoesgaming

**I'm submitting a ...**
  - [ ] bug fix
  - [x] feature enhancement
  - [ ] deprecation or removal
  - [ ] refactor

# Description
Allow compatibility with play.integer32.com and play.rust-lang.org

# Motivation
Allows people to use `uuid` in interactive code snippets

# Tests
Current tests passing

# Related Issue(s)
Supercedes #155
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