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

remove runtime parsing #6

Closed
wants to merge 5 commits into from
Closed

remove runtime parsing #6

wants to merge 5 commits into from

Conversation

chpio
Copy link
Contributor

@chpio chpio commented May 4, 2017

i removed the runtime parsing function. reasons for that:

  • parse_str "could" fail at runtime
  • parse_str can only accept &'static strs, so the range of use is reduced anyway to static strings
  • parse_str is only used by the tests

also changed:

  • removed 2 benches, because they tested parse_str perf
  • docs added

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.8%) to 96.203% when pulling d217e67 on chpio:master into e0de11c on CleverCloud:master.

2 similar comments
@coveralls
Copy link

coveralls commented May 4, 2017

Coverage Status

Coverage decreased (-3.8%) to 96.203% when pulling d217e67 on chpio:master into e0de11c on CleverCloud:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.8%) to 96.203% when pulling d217e67 on chpio:master into e0de11c on CleverCloud:master.

@coveralls
Copy link

coveralls commented May 4, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5773827 on chpio:master into e0de11c on CleverCloud:master.

this was needed just for testing, so whatever...
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fbd7131 on chpio:master into e0de11c on CleverCloud:master.

1 similar comment
@coveralls
Copy link

coveralls commented May 7, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling fbd7131 on chpio:master into e0de11c on CleverCloud:master.

@coveralls
Copy link

coveralls commented May 8, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 7541222 on chpio:master into e0de11c on CleverCloud:master.

@coveralls
Copy link

coveralls commented May 8, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a35e415 on chpio:master into e0de11c on CleverCloud:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a35e415 on chpio:master into e0de11c on CleverCloud:master.

@coveralls
Copy link

coveralls commented May 8, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a35e415 on chpio:master into e0de11c on CleverCloud:master.

@coveralls
Copy link

coveralls commented May 8, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling cb100d7 on chpio:master into e0de11c on CleverCloud:master.

@judu
Copy link
Member

judu commented May 8, 2017

Thanks!
I'm gonna publish the last version today.
Just wondering: why do you put so much in that (quite simplistic) lib? It's not bothering us at all, but for us it's just a small lib used by an internal project AND used to train an intern in a funny way.

@chpio
Copy link
Contributor Author

chpio commented May 8, 2017

im not using it right now, but i need this in the future

@Keruspe
Copy link
Contributor

Keruspe commented May 8, 2017

What's the problem with parse_str that can fail?
Why adding all those clone in the test?
I think it's much clearer with parse_str.

@Keruspe
Copy link
Contributor

Keruspe commented May 8, 2017

I'm fine with adding more api (such as the tuple one) and more tests with it but I think current sets should stay as is and parse_str should stay too

@chpio
Copy link
Contributor Author

chpio commented May 8, 2017

What's the problem with parse_str that can fail?
I think it's much clearer with parse_str.

parse_str can fail if you put something like this str into it: "Busybulbasaur". But this is completely unnecessary as parse_str can only work on 'static strs and thus the input is known at compile time.

Why adding all those clone in the test?

the from is impled for (&static str, &static str) but you get an &(&static str, &static str) thus the conversion.

@Keruspe
Copy link
Contributor

Keruspe commented May 8, 2017

I actually think that both parse_str and the tuple form should fail more often, when the input is not present in the arrays of pokemon and adjectives

@chpio
Copy link
Contributor Author

chpio commented May 8, 2017

yeah, thought about that too, but discarded that idea because of the complexity. you would need two large match expressions for the pokemons and adjectives, it's actually doable if done with macros, but again: i dont think it's worth the effort for a feature which is used purely for testing.

@Keruspe
Copy link
Contributor

Keruspe commented May 10, 2017

After internal discussions we actually ported the tests to to_string(), this makes them stick closer to what the project is supposed to actually do. I thus dropped parse_str as it was no longer needed.

Your doc improvements are nice though, care to rebase those?
Wrt the tuples stuff, i'm fine with adding these as well if you need them, alongside with new tests, leaving existing ones as is

@chpio
Copy link
Contributor Author

chpio commented May 10, 2017

What's with the benches? i also removed all eq benches in this pr

@chpio chpio closed this May 10, 2017
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.

None yet

4 participants