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

feat(core): use Cairo proc macros #2710

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

remybar
Copy link
Contributor

@remybar remybar commented Nov 21, 2024

Description

Use Cairo proc macros to replace the dojo-lang plugin.

Tests

  • Yes
  • No, because they aren't needed
  • No, because I need help

Added to documentation?

  • README.md
  • Dojo Book
  • No documentation needed

Checklist

  • I've formatted my code (scripts/prettier.sh, scripts/rust_fmt.sh, scripts/cairo_fmt.sh)
  • I've linted my code (scripts/clippy.sh, scripts/docs.sh)
  • I've commented my code
  • I've requested a review after addressing the comments

@remybar remybar changed the title wip feat(core): use Cairo proc macros Nov 21, 2024
@remybar remybar force-pushed the feat-proc_macros branch 5 times, most recently from c12b8b6 to 630ed68 Compare November 25, 2024 15:14
Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Looking great @remybar!

For the renaming of attributes, as discussed, we should check first we could help to PR this on the scarb side. Shouldn't be too much, and a bit unlock to avoid renaming everything.

Very nice for the new testing suite, this will be a good addition.

Also, Starknet Foundry should be usable at this point. If yes, let's add a new core-foundry-test crate to actually expose the same utilities we have in the core-cairo-test but for Starknet Foundry context.

@remybar remybar force-pushed the feat-proc_macros branch 3 times, most recently from 03cb1c3 to 590b2ee Compare November 27, 2024 15:51
@remybar remybar force-pushed the feat-proc_macros branch 6 times, most recently from 0b4c327 to 3dba3c5 Compare December 5, 2024 15:50
@@ -117,7 +117,7 @@ jobs:
scarb --manifest-path examples/spawn-and-move/Scarb.toml fmt --check
scarb --manifest-path examples/simple/Scarb.toml fmt --check
scarb --manifest-path crates/dojo/core/Scarb.toml fmt --check
scarb --manifest-path crates/dojo/core-cairo-test/Scarb.toml fmt --check
scarb --manifest-path crates/dojo/core-foundry-test/Scarb.toml fmt --check
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if we should keep both during a time? Since everybody may not migrate to foundry (at least for now?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But proc macros are disabled with the cairo test runner, so if we keep core-cairo-test, I think we also have to keep the Dojo plugin 😕 I don't know if it's possible to mix both plugin and proc macros ?

bin/dojo-language-server/src/main.rs Outdated Show resolved Hide resolved
#[arg(long, default_value = "none")]
profiler_mode: ProfilerMode,
/// Should we run the profiler.
#[arg(long, default_value_t = false)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

No more profiler mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bin/sozo/src/commands/test.rs Outdated Show resolved Hide resolved
@@ -69,6 +69,7 @@ ARG DOJO_VERSION=stable
ARG BUILD_TYPE=default
RUN curl -L https://install.dojoengine.org | bash
RUN curl --proto '=https' --tlsv1.2 -sSf https://docs.swmansion.com/scarb/install.sh | bash
RUN curl -L https://raw.githubusercontent.com/foundry-rs/starknet-foundry/master/scripts/install.sh | bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will do a run on a dojo branch to generate a container that you can use to work with it.

We're about to bump cairo, will do this right after. Like so you can also update to cairo 2.9. 👍

@glihm glihm added the blocked This issue is blocked due to some other issues label Jan 3, 2025
@glihm
Copy link
Collaborator

glihm commented Jan 3, 2025

Blocked by upstream scarb changes to ensure:

  • support to rename attributes to follow current dojo::model syntax.
  • correct diagnostics to actually show the line with the error (currently not mapped).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue is blocked due to some other issues contributor dojo-lang
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants