-
Notifications
You must be signed in to change notification settings - Fork 579
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
LS: Proc macro - scarb code #6638
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/scarb/regular.rs
line 30 at r1 (raw file):
} fn calculate_metadata(db: &dyn SyntaxGroup, item_ast: ast::ModuleItem) -> TokenStreamMetadata {
This should be lower (public first)
6b63f90
to
55303af
Compare
94ce188
to
3709348
Compare
55303af
to
e49ac1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 7 files reviewed, all discussions resolved (waiting on @Arcticae, @mkaput, @orizi, and @piotmag769)
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/scarb/regular.rs
line 30 at r1 (raw file):
Previously, piotmag769 (Piotr Magiera) wrote…
This should be lower (public first)
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Arcticae, @mkaput, and @orizi)
e49ac1f
to
1790b0d
Compare
f7c5016
to
bedb75d
Compare
f1a8998
to
cb0d2d5
Compare
4966290
to
1c0f71e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r4, all commit messages.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)
crates/cairo-lang-language-server/Cargo.toml
line 29 at r4 (raw file):
cairo-lang-utils = { path = "../cairo-lang-utils", version = "~2.8.4" } cairo-lang-macro = "0.1.1" # Used in proc-macro code copied from scarb.
These comments are unnecessary imho: if we use these deps, then we use them, and comments break easily sorting deps
1c0f71e
to
ae851a5
Compare
cb0d2d5
to
4763ba9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @mkaput, @orizi, and @piotmag769)
crates/cairo-lang-language-server/Cargo.toml
line 29 at r4 (raw file):
Previously, mkaput (Marek Kaput) wrote…
These comments are unnecessary imho: if we use these deps, then we use them, and comments break easily sorting deps
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r2, 2 of 2 files at r4.
Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on @Draggu, @mkaput, @orizi, and @piotmag769)
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/scarb/inline.rs
line 12 at r4 (raw file):
use crate::lang::proc_macros::db::ProcMacroCacheGroup; // scarb code
I would mark where it comes from, for easier exploration and maintainability (perhaps a git permalink also would be nice)
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/scarb/mod.rs
line 11 at r4 (raw file):
pub mod regular; // Code from scarb.
Same as above
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/scarb/regular.rs
line 20 at r4 (raw file):
use crate::lang::proc_macros::db::ProcMacroCacheGroup; // scarb code
Same as above
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs
line 12 at r4 (raw file):
// Keep it private. mod downcast; // Code from scarb.
Let's not describe it on import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Draggu and @mkaput)
4763ba9
to
597ba75
Compare
597ba75
to
e1bfa43
Compare
4e8933a
to
85e1ce0
Compare
845f36d
to
9973420
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 7 files reviewed, 5 unresolved discussions (waiting on @Arcticae, @integraledelebesgue, @mkaput, and @piotmag769)
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/mod.rs
line 12 at r4 (raw file):
Previously, Arcticae (Tomasz Rejowski) wrote…
Let's not describe it on import
I believe this was comment to previous comment.
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/scarb/inline.rs
line 12 at r4 (raw file):
Previously, Arcticae (Tomasz Rejowski) wrote…
I would mark where it comes from, for easier exploration and maintainability (perhaps a git permalink also would be nice)
Done.
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/scarb/mod.rs
line 11 at r4 (raw file):
Previously, Arcticae (Tomasz Rejowski) wrote…
Same as above
Done.
crates/cairo-lang-language-server/src/lang/proc_macros/plugins/scarb/regular.rs
line 20 at r4 (raw file):
Previously, Arcticae (Tomasz Rejowski) wrote…
Same as above
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 7 files at r6, 1 of 1 files at r7, 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae and @integraledelebesgue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r8.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)
9973420
to
492629a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)
ab322b6
to
1bcc9a9
Compare
89ee80c
to
68b1527
Compare
1631ba2
to
3faadef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @integraledelebesgue)
68b1527
to
36015d2
Compare
36015d2
to
b65ebe2
Compare
163fa24
to
d9f751a
Compare
Code calling dylib is replaced with one calling proc-macro-server. Also `aux_data` is removed as it is useless for LS. commit-id:381d1f35
b65ebe2
to
a55c8a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 4 files at r8, 1 of 2 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Code calling dylib is replaced with one calling proc-macro-server. Also
aux_data
is removed as it is useless for LS.Stack: