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 --acyclic option #184

Merged
merged 5 commits into from
May 31, 2023
Merged

Add --acyclic option #184

merged 5 commits into from
May 31, 2023

Conversation

smoelius
Copy link
Contributor

@regexident Are you still open to adding #18?

I tried to implement it in a way that was faithful to #18 (comment).

For example, here is the output from a run on cargo:

$ cargo modules generate graph --acyclic --layout=none --package cargo --lib
Error: Circular dependency between `cargo::util::config` and `cargo::util::toml`.

┌> cargo::util::config
│  └─> cargo::util::auth
│      └─> cargo::util
│          └─> cargo::util::toml
└──────────────┘

Nits are welcome, though.

@smoelius
Copy link
Contributor Author

Re the rustfmt failure: I included a rustfmt.toml file to make it easier to diff tri_color.rs against https://github.com/rust-lang/rust/blob/925dc37313853f15dc21e42dc869b024fe488ef3/compiler/rustc_data_structures/src/graph/iterate/mod.rs.

Is that silly? Should we just live with an ugly diff?

I'm not sure what to make of "test with minimal versions" failure.

@smoelius
Copy link
Contributor Author

Also, I added an "MIT OR Apache-2.0" license header to tri_color.rs. But based on my understanding of those licenses, that file could be licensed MPL.

@smoelius
Copy link
Contributor Author

I'm not sure what to make of "test with minimal versions" failure.

Ah. It's failing because of bitvec dependencies:

@smoelius
Copy link
Contributor Author

I'm going to hold back on trying to push further fixes. Please let me know how you would like to proceed.

Copy link
Owner

@regexident regexident left a comment

Choose a reason for hiding this comment

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

Hi @smoelius, thank you for taking the time to work on this!

I included a rustfmt.toml file to make it easier to diff tri_color.rs […] Is that silly? Should we just live with an ugly diff?

I generally prefer to keep rustfmt's config unchanged, but for the sake of diffing with upstream this seems like a reasonable exception to make.

I would however appreciate if you could move tri_color.rs and rustfmt.toml into a crate::graph::cycles module to limit the effect of rustfmt.toml to just crate::graph::cycles::tri_color and not all of crate::graph::*.

The .github/configs/skywalking-eyes.yml file will also require an exception for the tri_color.rs file:

  paths:
    - "src/**/*.rs"
  paths-ignore: 👈
    - "src/graph/cycles/tri_color.rs" 👈

For the minimal-versions failure you may want to add the following to Cargo.toml at line 44:

# minimal versions
serde_repr = "0.1.12"
wyz = "0.5.1" 👈

src/graph/tri_color.rs Outdated Show resolved Hide resolved
smoelius and others added 2 commits May 22, 2023 19:04
Co-authored-by: Vincent Esche <138017+regexident@users.noreply.github.com>
@smoelius
Copy link
Contributor Author

I would however appreciate if you could move tri_color.rs and rustfmt.toml into a crate::graph::cycles module to limit the effect of rustfmt.toml to just crate::graph::cycles::tri_color and not all of crate::graph::*.

That absolutely should have occurred to me.

Please tell me what else you think needs to be adjusted.

@regexident
Copy link
Owner

Please tell me what else you think needs to be adjusted.

The only other thing that comes to my mind right now be adding a comment to rustfmt.toml to give some context as to why it's there.

@smoelius
Copy link
Contributor Author

The only other thing that comes to my mind right now be adding a comment to rustfmt.toml to give some context as to why it's there.

Done. No worries if you think of something else.

@regexident
Copy link
Owner

regexident commented May 31, 2023

Thanks for working on this @smoelius!

A v0.9.0 has just been published with it.

@regexident regexident merged commit 00debf4 into regexident:main May 31, 2023
6 checks passed
@smoelius smoelius deleted the acyclic branch May 31, 2023 08:56
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

2 participants