Split up parse_str.rs file #236

New issue

Closed

opened 2018-05-20 20:39:30 -05:00 by kinggoesgaming · 5 comments

kinggoesgaming commented

2018-05-20 20:39:30 -05:00

(Migrated from github.com)

Description

The benchmarks file has grown to not just parse_str function. As with the lib.rs, this is important to be split.

Other

https://github.com/uuid-rs/uuid/blob/master/benches/parse_str.rs

#124

# Description The benchmarks file has grown to not just `parse_str` function. As with the `lib.rs`, this is important to be split. # Other * https://github.com/uuid-rs/uuid/blob/master/benches/parse_str.rs #124

murrayjbrown commented

2018-08-30 19:03:24 -05:00

(Migrated from github.com)

@Dylan-DPC You'd pointed me to this issue as a starter Rust project a while ago on IRC. Sorry for the delay but I thought it worthwhile to prepare by taking another tour through the Rust Book. ;-)

I'm having trouble understanding this issue: It does not parse (if you'll excuse the pun) — the English description above, that is. I suspect it should rather read as follows:

The benchmarks file has grown, too, not just parse_str function.

Grammar aside, I still don't get what is needed. Can you please clarify or elaborate? What is meant here by split?

@Dylan-DPC You'd pointed me to this issue as a starter Rust project a while ago on IRC. Sorry for the delay but I thought it worthwhile to prepare by taking another tour through the Rust Book. ;-) I'm having trouble understanding this issue: It does not parse (if you'll excuse the pun) — the English description above, that is. I suspect it should rather read as follows: > The benchmarks file has grown, too, not just parse_str function. Grammar aside, I still don't get what is needed. Can you please clarify or elaborate? What is meant here by _split?_

kinggoesgaming commented

2018-08-31 07:53:34 -05:00

(Migrated from github.com)

Hi @murrayjbrown,

Welcome back!!

I just reread my own issue report and indeed it could worded better.

Currently we have 2 files in benches/ folder, which until recently was one file. The number of various bench tests are growing in this one file. What we want is group of associated tests be moved into their own file, so that parse_str.rs just contains the benchmarks for parsing strings to uuid's.

I hope this clears things up. I am currently on my phone so can't really write an essay here but I will check on this :)

Hi @murrayjbrown, Welcome back!! I just reread my own issue report and indeed it could worded better. Currently we have 2 files in benches/ folder, which until recently was one file. The number of various bench tests are growing in this one file. What we want is group of associated tests be moved into their own file, so that parse_str.rs just contains the benchmarks for parsing strings to uuid's. I hope this clears things up. I am currently on my phone so can't really write an essay here but I will check on this :)

murrayjbrown commented

2018-08-31 14:53:43 -05:00

(Migrated from github.com)

Hi @kinggoesgaming, thanks! That's much clearer. However, it appears to me that all the bench tests in benches/parse_str.rs are related to parsing strings to uuids; it's just that some appear to be valid normal cases, while others are abnormal, both error cases and valid edge cases. So, that's how I propose to split them (normal/abnormal), unless I hear differently.

Hi @kinggoesgaming, thanks! That's much clearer. However, it appears to me that all the bench tests in _benches/parse_str.rs_ are related to parsing strings to uuids; it's just that some appear to be valid normal cases, while others are abnormal, both error cases and valid edge cases. So, that's how I propose to split them (normal/abnormal), unless I hear differently.

kinggoesgaming commented

2018-08-31 15:34:45 -05:00

(Migrated from github.com)

Ok so just had proper look at the file and I think we can split the file into 3:

valid_parse_str.rs
invalid_parse_str.rs
slog_support/parse_str.rs

The last one is in accordance with the src/ folder structure, where dependencies have a separate module to handle, here we just use a file instead.

As this will may alter the benchmark results, please paste before/after results here or in a comment of the PR that you do create (for keeping track).

And thanks for picking up on this issue :)

Ok so just had proper look at the file and I think we can split the file into 3: * `valid_parse_str.rs` * `invalid_parse_str.rs` * `slog_support/parse_str.rs` The last one is in accordance with the `src/` folder structure, where dependencies have a separate module to handle, here we just use a file instead. As this will may alter the benchmark results, please paste before/after results here or in a comment of the PR that you do create (for keeping track). And thanks for picking up on this issue :)

👍 1

murrayjbrown commented

2018-08-31 15:48:30 -05:00

(Migrated from github.com)