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

fix: underscore crate name #38

Merged
merged 5 commits into from
Aug 17, 2024
Merged

Conversation

EvolveArt
Copy link
Contributor

@EvolveArt EvolveArt commented Aug 12, 2024

Resolves #37

Changes

  • Adds the replace call as mentioned in the issue's comments
  • Includes corresponding unit and acceptance test

@DenuxPlays
Copy link
Collaborator

Does normal crates name liked normal-crate still work?
I am not sure if this is tested?

Copy link
Collaborator

@DenuxPlays DenuxPlays left a comment

Choose a reason for hiding this comment

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

Unit test looks fine.
But I am missing an acceptance test.
I think we already have a test crate in the tests folder which could be extended.

@EvolveArt
Copy link
Contributor Author

EvolveArt commented Aug 13, 2024

Unit test looks fine. But I am missing an acceptance test. I think we already have a test crate in the tests folder which could be extended.

Should I create another create-segment-path for instance ? or just a sub-folder ?

EDIT: the issue is on the crate name, so the only acceptance test that could be done is by creating a new test crate
EDIT 2: the tests folder doesn't seem to be compiled anywhere

@DenuxPlays
Copy link
Collaborator

Unit test looks fine. But I am missing an acceptance test. I think we already have a test crate in the tests folder which could be extended.

Should I create another create-segment-path for instance ? or just a sub-folder ?

EDIT: the issue is on the crate name, so the only acceptance test that could be done is by creating a new test crate

EDIT 2: the tests folder doesn't seem to be compiled anywhere

Yes just create another test crate.

I noticed this too so I already created a fix: #39

@EvolveArt
Copy link
Contributor Author

Unit test looks fine. But I am missing an acceptance test. I think we already have a test crate in the tests folder which could be extended.

Should I create another create-segment-path for instance ? or just a sub-folder ?
EDIT: the issue is on the crate name, so the only acceptance test that could be done is by creating a new test crate
EDIT 2: the tests folder doesn't seem to be compiled anywhere

Yes just create another test crate.

I noticed this too so I already created a fix: #39

Ok so I will rebase once your fix is merged

@DenuxPlays
Copy link
Collaborator

@EvolveArt

PRs have been merged you can now merge main into your branch and create the acceptance test

@EvolveArt
Copy link
Contributor Author

@EvolveArt

PRs have been merged you can now merge main into your branch and create the acceptance test

Done, you can check. Not sure if that's the best way but it does provide a correct acceptance test.
I think acceptance tests could be refactored in another PR.

@@ -1,5 +1,5 @@
[workspace]
members = ["crate_segment_path"]
members = ["crate_segment_path", "new_crate_segment_path"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
members = ["crate_segment_path", "new_crate_segment_path"]
members = ["crate_segment_path", "folder_in_src"]

utoipauto.workspace = true
utoipauto-lib-test-new = { path = "../new_crate_segment_path" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
utoipauto-lib-test-new = { path = "../new_crate_segment_path" }
folder_in_src = { path = "../folder_in_src" }

Copy link
Collaborator

@DenuxPlays DenuxPlays left a comment

Choose a reason for hiding this comment

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

Please rename the folder and the name to folder_in_src
The name is more descriptive.

Otherwise your pr looks pretty good.
Thank you for your work :)

@EvolveArt
Copy link
Contributor Author

Please rename the folder and the name to folder_in_src The name is more descriptive.

Otherwise your pr looks pretty good. Thank you for your work :)

Done :) looking forward to help more, this crate has been really useful to simplify our utoipa doc generation

Copy link
Collaborator

@DenuxPlays DenuxPlays left a comment

Choose a reason for hiding this comment

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

Looks good to me

@DenuxPlays
Copy link
Collaborator

DenuxPlays commented Aug 16, 2024

Note:

I am not sure if we should introduce a "main" testing crate where we could merge all things that need to be tested across multiple crates (like this pr).
But this has nothing todo with this pr.
Just a small note from me.

Note 2:

I do have a plan for creating a larger test crate that tests multiple things maybe we can combine the ideas.

Again these notes have nothing todo with this pr!

@DenuxPlays
Copy link
Collaborator

@EvolveArt Could you rebase your pr one more time?
I fixed the ci a bit

@EvolveArt
Copy link
Contributor Author

@EvolveArt Could you rebase your pr one more time? I fixed the ci a bit

done

@ProbablyClem
Copy link
Owner

Thanks guy for your work

@ProbablyClem ProbablyClem merged commit eb04cba into ProbablyClem:main Aug 17, 2024
7 checks passed
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.

Does discovery account for dash / underscore difference in path vs module name?
3 participants