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: use core instead of std #283

Merged
merged 7 commits into from
Nov 17, 2024
Merged

refactor: use core instead of std #283

merged 7 commits into from
Nov 17, 2024

Conversation

rnbguy
Copy link
Contributor

@rnbguy rnbguy commented Nov 13, 2024

Closes: #282

@rnbguy rnbguy changed the title use core instead of std refactor: use core instead of std Nov 13, 2024
@rnbguy
Copy link
Contributor Author

rnbguy commented Nov 13, 2024

It would be nice if my regression can be added somewhere to be tested under no_std during CI jobs.

I can add some code for this -- please let me know how you want this to be.

@la10736
Copy link
Owner

la10736 commented Nov 13, 2024

I don't think we need to change it in all creates but just in rstest. The procedural macros ones and the others can still use std.

@la10736
Copy link
Owner

la10736 commented Nov 13, 2024

Maybe a little deep look to the rendering module can be useful, but just where it render some code that directly reference std

@rnbguy
Copy link
Contributor Author

rnbguy commented Nov 13, 2024

hey, I know where exactly I have to make changes only for my issue.

(&&&Magic::<#arg_type>(std::marker::PhantomData)).magic_conversion(#fixture)

but, don't you think that it would make sense to use core over std wherever it is possible ?

@la10736
Copy link
Owner

la10736 commented Nov 13, 2024

What I mean is that just the library crate should be nostd bceause it's used by the expanded code. The code in rstest_macro crate is executed by the compiler and so can always use std library.

Now in rstest_macros::render code maybe there is some quoted code that use std path... we should just check it

@la10736
Copy link
Owner

la10736 commented Nov 13, 2024

Moreover a nice to have... end to end test that expose the issue and then is fixed by your code

@rnbguy
Copy link
Contributor Author

rnbguy commented Nov 14, 2024

I reverted the core usage from the old commit. I now use core only in rstest_macros/src/render and rstest/src/magic_conversion.

I can't make rstest/src/timeout no_std compatible -- as it requires mpsc and thread from std.

@la10736
Copy link
Owner

la10736 commented Nov 14, 2024

Sorry, I've answered before without put enough brain in it .... sorry.

As I already commented in the linked ticket the library rstest can still remain in std because it will be linked in an std compatible executable. Only the rendered code should use references from core instead std just because they can be compiled in a library that doesn't have the std crate in its namespace.

Anyway in a future I'll remove all references to common code and just reexport them in rstest ... this should be the best way

@rnbguy
Copy link
Contributor Author

rnbguy commented Nov 14, 2024

the library rstest can still remain in std because it will be linked in an std compatible executable.

Gotcha! I reverted the changes in rstest crate. the changes are only in rstest_macros crate at the generate codes.

Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

Can you fill che changelog file and try to add an e2e test... Let me know if you need any hael for the e2e stuff

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for my wrong advice... we didn't need them

@la10736 la10736 self-requested a review November 17, 2024 14:17
@la10736
Copy link
Owner

la10736 commented Nov 17, 2024

Ok, I merge it. But I'll add at least a e2e test before publish it

@la10736 la10736 merged commit 6bbd44b into la10736:master Nov 17, 2024
2 checks passed
@rnbguy
Copy link
Contributor Author

rnbguy commented Nov 17, 2024

Hey, many thanks for merging this. Sorry, I couldn't get some time to add the changelog entry and the e2e test.

@la10736
Copy link
Owner

la10736 commented Nov 17, 2024 via email

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.

magic conversion fails to compile under no_std
2 participants