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

Restructure the crates #549

Merged
merged 5 commits into from
Sep 20, 2024
Merged

Conversation

adamperkowski
Copy link
Contributor

@adamperkowski adamperkowski commented Sep 19, 2024

This is a big big PR. Be careful.

Type of Change

  • Refactoring

Description

Restructured the project to be more intuitive and to be able to publish on crates.io.

  • Version 0.1.0 -> 24.9.19
  • Moved tabs dir to core/tabs so linutil_core can access it.

Resolves #333

Testing

Compiles. Can publish.

Additional Information

C O N F L I C T S .

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no errors/warnings/merge conflicts.

@adamperkowski adamperkowski marked this pull request as ready for review September 19, 2024 22:33
@adamperkowski
Copy link
Contributor Author

adamperkowski commented Sep 19, 2024

@ChrisTitusTech This is ready to merge. There are literally no PRs open now, I'd merge ASAP. And yes, this is the only (correct) way to do this.

@lj3954
Copy link
Contributor

lj3954 commented Sep 19, 2024

This makes the codebase more complicated, and requires reversion or major modifications if a GUI is to be developed within this repository (which is on the roadmap).

Is it not possible to include files within a parent directory within Cargo.toml?

@adamperkowski
Copy link
Contributor Author

adamperkowski commented Sep 19, 2024

This makes the codebase more complicated

I think the opposite. Now we only have one crate (linutil) depending on a child crate core. It's simplier in my opinion.

Is it not possible to include files within a parent directory within Cargo.toml?

No, unfortunately it's not. Already researched it. This seems to be the only way.

Also, I've seen some major projects (nushell for example) do the exact same.

@lj3954
Copy link
Contributor

lj3954 commented Sep 19, 2024

I think the opposite. Now we only have one crate (linutil) depending on a child crate core. It's simplier in my opinion.

Once again, a GUI is on the roadmap. A Cargo.toml including both the data for building a binary & workspace details in one also is not particularly simple.

No, unfortunately it's not. Already researched it. This seems to be the only way.

Then, moving tabs into core will be necessary. However, moving the tui into the root directory won't accomplish anything necessary.
Here's the note cargo gives you when you have dependencies relying on a path.

Note: The published dependency will use the version from crates.io,
the `path` specification will be removed from the dependency declaration.

@nnyyxxxx

This comment was marked as outdated.

@adamperkowski
Copy link
Contributor Author

adamperkowski commented Sep 19, 2024

A Cargo.toml including both the data for building a binary & workspace details in one also is not particularly simple.

I don't understand your point here. What exactly is not simple? It's done perfectly in this PR.

Then, moving tabs into core will be necessary. However, moving the tui into the root directory won't accomplish anything necessary.

Consider adding " - my personal opinion".

Here's the note cargo gives you when you have dependencies relying on a path.

linutil_core is going to be published on crates.io aswell. There is no other way.

@lj3954
Copy link
Contributor

lj3954 commented Sep 19, 2024

" - my personal opinion". Consider adding this to the sentence.

My personal opinion is that it makes the codebase more complicated. You disagree. It's a fact that it's not a necessary change. And it's a fact that the structure will need to be changed once more when work on a GUI, which is on the roadmap, begins.

Yes, linutil core will be published on crates.io. That's how the TUI will be able to depend on it. What I was noting is that moving the tui outside of its own directory is not a necessity.

@adamperkowski
Copy link
Contributor Author

It's a fact that it's not a necessary change.

It's neccessary to do this if we want to publish the crate.

What I was noting is that moving the tui outside of its own directory is not a necessity.

We NEED a binary target in the main crate to make this possible. linutil_tui is the main executable.

@lj3954
Copy link
Contributor

lj3954 commented Sep 19, 2024

We NEED a binary target in the main crate to make this possible. linutil_tui is the main executable.

cargo publish -p linutil_tui would work perfectly whilst leaving TUI code in its own directory. Once again, a GUI is on the roadmap. A PR which makes several changes that must be reverted to begin GUI development in this repository should not be merged before those changes are dropped.

Moving tabs to core/, if necessary as you state, should occur. Versioning is also necessary, although I'm not completely sure why semver can't be used. But the last change, moving TUI into the root of the repository, must not be made.

@lj3954
Copy link
Contributor

lj3954 commented Sep 19, 2024

lj3954@aabac38

This should fix the issue.

@adamperkowski
Copy link
Contributor Author

cargo publish -p linutil_tui would work perfectly whilst leaving TUI code in its own directory. Once again, a GUI is on the roadmap.

I understand that a GUI is on the roadmap perfectly. If we're looking at this from a GUI standpoint, your approach does make more sense. Committing the changes now.

I'm not completely sure why semver can't be used

Chris says that he wants to stick with date versioning. I know this is a worse idea, I suggested using semver aswell.

@ChrisTitusTech
Copy link
Owner

Let me wrap my head around semver if it is better I'll figure out a way to implement it and dump my archaic date versioning.

Co-authored-by: Liam <lj3954@users.noreply.github.com>
@ChrisTitusTech ChrisTitusTech merged commit 216f1a4 into ChrisTitusTech:main Sep 20, 2024
3 checks passed
@adamperkowski adamperkowski deleted the crates-io branch September 20, 2024 00:12
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.

Crates.io & AUR packages
4 participants