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

refactor tests and impls into distinct modules #36

Merged
merged 22 commits into from
Aug 29, 2023

Conversation

chris-ha458
Copy link
Contributor

I first started by moving all tests to tests/tests.rs
This was to ensure maximum isolation to see what kind of testing and usage is possible without access to private details.

Then, for any tests that do not compile, I moved them into a separate file called src/unit_tests.rs
This allowed for a more cleaner separation imports as well.

Feel free to comment on any aspects of this PR including separation of integration tests and unit tests, filenames or paths, or if you feel like any tests were misplaced(should be in integration but included in unit or vice versa).
I recognize that a lot of this would be a stylistic choice and I am not strongly tied to the current organisation.

When we find an acceptable organisation, I can proceed to further refactor by separating some traits from lib.rs into a separate traits.rs or counter.rs (if this is indeed something you'd be willing to see in the first place)

Copy link
Owner

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Interesting start, and a good idea in principle.

With regard to further splitting out impls, I'd propose an organization like this:

  • src/impls/index.rs: Index, IndexMut
  • src/impls/add.rs: Add, AddAssign
  • src/impls/intersection.rs: BitAnd, BitAndAssign
  • src/impls/union.rs: BitOr, BitOrAssign
  • and so on

src/lib.rs Outdated Show resolved Hide resolved
src/unit_tests.rs Show resolved Hide resolved
src/unit_tests.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@chris-ha458
Copy link
Contributor Author

Since i've reflected all changes (except the lint related one) and tests are running again I'll try the splitting out impls, one by one.

src/impls/add.rs Outdated Show resolved Hide resolved
@chris-ha458
Copy link
Contributor Author

so now the add{_iterable,_self} sub{_iterable,_self} index intersection union are separated but many are still left.

From here I could think of
init.rs or create.rs where the new,default or other impl that create new Counters.

@coriolinus
Copy link
Owner

My instinct for now is to leave all the impl Counter blocks in lib.rs. If we can find a place where there are two blocks with identical bounds, we could unify those, but I want to see how big lib.rs is with the trait implementations factored out before deciding to break up the core item implementations.

You've seen the pattern, as far as splitting up the trait implementations. Just focus on the semantics of the operations, and make a module for each. I'll look it over when I have the chance, and comment if I see something I don't like.

This one is difficult since there are several
`impl<T, N> Counter<T, N>` blocks that also create new counters.
@coriolinus coriolinus changed the title Tests refactor refactor tests and impls into distinct modules Aug 29, 2023
@chris-ha458
Copy link
Contributor Author

I think even deref.rs might have gone too far.
Also i don't think i haven't seen any blocks with identical bounds.

@chris-ha458
Copy link
Contributor Author

I would like to move some impl Counter that creates new counters into create.rs. But i tried to stay within your instinct

@coriolinus
Copy link
Owner

deref.rs is fine for me. Stays consistent.

What if instead of a single create.rs, there was a from_iterator.rs handling that case, and a new.rs handling creation of new empty counters (including Default)? That way, the module is handling only a single modality of creation.

src/impls/create.rs Outdated Show resolved Hide resolved
Copy link
Owner

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

I'm happy with this. The module structure looks much more balanced now:

$ fd -g '*.rs' -x wc -l | sort -rn
Tue 29 Aug 2023 03:44:11 PM CEST
599 ./src/lib.rs
235 ./src/unit_tests.rs
186 ./tests/tests.rs
91 ./src/impls/index.rs
84 ./src/impls/union.rs
82 ./src/impls/sub_self.rs
77 ./src/impls/into_iterator.rs
77 ./src/impls/extend.rs
74 ./src/impls/from_iterator.rs
71 ./src/impls/intersection.rs
62 ./src/impls/add_self.rs
57 ./src/impls/sub_iterable.rs
55 ./src/impls/add_iterable.rs
33 ./src/impls/create.rs
26 ./src/impls/deref.rs
12 ./src/impls.rs

There's one more change that might reasonably fit into this pr: externalize the module documentation. That would subtract almost 300 lines from lib.rs. However, I am ambivalent about it; you can choose to make that change, or not.

@chris-ha458
Copy link
Contributor Author

If all else is well, lets merge for now.
I'll look into 1990-external-doc-attribute that you've linked. Depending on the perceived difficulty/necessity, I will do that or bench with criterion next.

@coriolinus coriolinus merged commit f3bbdd7 into coriolinus:master Aug 29, 2023
1 check passed
@chris-ha458 chris-ha458 deleted the refactor branch August 29, 2023 14:35
ruancomelli referenced this pull request in ruancomelli/learning-rust Jul 1, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [counter](https://togithub.com/coriolinus/counter-rs) | dependencies |
minor | `0.5.7` -> `0.6.0` |

---

### Release Notes

<details>
<summary>coriolinus/counter-rs (counter)</summary>

###
[`v0.6.0`](https://togithub.com/coriolinus/counter-rs/releases/tag/v0.6.0)

[Compare
Source](https://togithub.com/coriolinus/counter-rs/compare/v0.5.7...v0.6.0)

#### What's Changed

- update edition, add `counter` keyword by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/34](https://togithub.com/coriolinus/counter-rs/pull/34)
- refactor tests and impls into distinct modules by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/36](https://togithub.com/coriolinus/counter-rs/pull/36)
- small doc formatting by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/38](https://togithub.com/coriolinus/counter-rs/pull/38)
- deprecate `init` method by
[@&#8203;coriolinus](https://togithub.com/coriolinus) in
[https://github.com/coriolinus/counter-rs/pull/41](https://togithub.com/coriolinus/counter-rs/pull/41)
- do not use deprecated `init` method by
[@&#8203;coriolinus](https://togithub.com/coriolinus) in
[https://github.com/coriolinus/counter-rs/pull/42](https://togithub.com/coriolinus/counter-rs/pull/42)
- With capacity by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/40](https://togithub.com/coriolinus/counter-rs/pull/40)
- Clippy fixes by
[@&#8203;chris-ha458](https://togithub.com/chris-ha458) in
[https://github.com/coriolinus/counter-rs/pull/43](https://togithub.com/coriolinus/counter-rs/pull/43)
- Implement Serialize and Deserialize by
[@&#8203;matthewmcintire-savantx](https://togithub.com/matthewmcintire-savantx)
in
[https://github.com/coriolinus/counter-rs/pull/46](https://togithub.com/coriolinus/counter-rs/pull/46)

#### New Contributors

- [@&#8203;chris-ha458](https://togithub.com/chris-ha458) made their
first contribution in
[https://github.com/coriolinus/counter-rs/pull/34](https://togithub.com/coriolinus/counter-rs/pull/34)
-
[@&#8203;matthewmcintire-savantx](https://togithub.com/matthewmcintire-savantx)
made their first contribution in
[https://github.com/coriolinus/counter-rs/pull/46](https://togithub.com/coriolinus/counter-rs/pull/46)

**Full Changelog**:
coriolinus/counter-rs@v0.5.7...v0.5.8

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/ruancomelli/learning-rust).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40MjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjQyMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJyZW5vdmF0ZSJdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

2 participants