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

chore(rust): Re-organize folder structure for Rust crates #10141

Merged
merged 7 commits into from
Jul 28, 2023
Merged

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Jul 28, 2023

I was looking at the ruff code base, and I am extremely jealous of the nice organization of their crates, and repo structure in general. I messed around a bit within our repo and came up with this.

I realize there is probably good reason for the repo to be set up the way it is right now, so please enlighten me! But I figured I'd just open a PR since I already had something working anyway.

Changes:

  • polars-lazy had two crates in its subfolders: polars-plan and polars-pipe. These have been moved up one level, so they are no longer under polars-lazy.
  • The main polars crate is moved down one level and lives next to the other crates.
  • The folder containing all crates has been renamed crates.
  • Update references in Cargo.toml files
  • Obviously no functional changes hidden in the 900+ files changed 😅

The main motivation for this change is to eliminate the pattern of 'sub-crates'. Something feels off about having a standard Rust crate setup with src, Cargo.toml, etc. and then have other folders outside the src folder with more source code in them. I also cannot find any examples of this pattern looking through some other Rust code bases or the Cargo docs.

I also expect this 'flat' structure to make it easier to configure tooling / Cargo for CI purposes.

I can imagine the motivation for this change is quite weak and doesn't warrant such a sweeping change, but I figured I'd throw it out there anyway.

Thoughts?

@github-actions github-actions bot added internal An internal refactor or improvement rust Related to Rust Polars labels Jul 28, 2023
@ritchie46
Copy link
Member

A counter example: https://github.com/datafuselabs/databend/tree

Polars-pipe and polars-plan are subcrates of polars-lazy and not accessible by any other crate. We can of-course make it flat.

My rationale was that crates that can be used by other polars crates are flat, and that sub-crates that merely serve as sort of modules for a higher crate are nested. Because, though you change the folder structure now, it still are sub-crates. ;)

@stinodego
Copy link
Member Author

A counter example: https://github.com/datafuselabs/databend/tree

Ah thanks! They have lots of crates 😅 but they don't really nest them - it's more like a folder structure with separate crates. Not crates-within-crates (with a rare exception).

though you change the folder structure now, it still are sub-crates

True, in the end the dependencies dictate whether something should be considered a sub-crate or not.

But following that logic, polars-sql depends on polars-plan directly, so it's not really a proper sub-crate of polars-lazy anymore.
And all crates were organized below the main polars crate, but some of those crates are referenced directly as dependencies in for example py-polars and some examples / contribution crates.

So to me it makes more sense to just get rid of the idea that folder structure indicates interdependency, and just organize it in a flat way. That also brings some uniformity to for example the path definition of dependencies in Cargo.tomls.

@ritchie46
Copy link
Member

Ok, no strong opinion here. Flat is easier to grok

@stinodego
Copy link
Member Author

stinodego commented Jul 28, 2023

My OCD will be so pleased 😄

Any preference for the main folder? It's crates now, could also be called rust or possibly something else. I think I like crates best.

I will leave this open for a bit for other maintainers to chime in, and if no objections pop up, rebase & merge it at the end of the day.

@universalmind303
Copy link
Collaborator

I share the same sentiments, flat is a bit easier to navigate.

@stinodego stinodego merged commit 128803b into main Jul 28, 2023
16 checks passed
@stinodego stinodego deleted the move-crates branch July 28, 2023 20:02
@ritchie46
Copy link
Member

@stinodego this broke the build.rs logic. Can you take a look.

The problem is that the nightly feature flag now isn't set anymore for all crates. When we nested under a global polars crate, all subcrates inherited the build.rs file.

This now break clippy locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants