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

LS: Send project model updates through channel #6734

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

piotmag769
Copy link
Collaborator

@piotmag769 piotmag769 commented Nov 25, 2024

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@piotmag769 piotmag769 force-pushed the spr/main/d41ad059 branch 3 times, most recently from 254db01 to 17bd99e Compare November 25, 2024 15:21
@piotmag769 piotmag769 changed the title LS: Send metadata through channel LS: Send project model updates through channel Nov 25, 2024
Copy link
Collaborator

@Draggu Draggu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @Arcticae, @integraledelebesgue, @mkaput, @orizi, and @piotmag769)


crates/cairo-lang-language-server/src/toolchain/scarb.rs line 130 at r1 (raw file):

        if !self.is_silent {
            self.notifier.notify::<ScarbResolvingFinish>(());
            self.notifier.notify::<ScarbMetadataFailed>(());

Why? You don't even check if it was successful or not.


crates/cairo-lang-language-server/src/project/mod.rs line 26 at r1 (raw file):

mod unmanaged_core_crate;

pub struct ProjectController {

Move Notifier and ScarbToolchain to the controller state to make it consistent with other controllers


crates/cairo-lang-language-server/src/project/mod.rs line 88 at r1 (raw file):

        };

        // TODO: do sth smart here

What is "sth smart"?

Copy link
Collaborator Author

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r2.
Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @Arcticae, @Draggu, @integraledelebesgue, @mkaput, and @orizi)


crates/cairo-lang-language-server/src/project/mod.rs line 26 at r1 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Move Notifier and ScarbToolchain to the controller state to make it consistent with other controllers

If it will be used in more than one function I will


crates/cairo-lang-language-server/src/project/mod.rs line 88 at r1 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

What is "sth smart"?

Done.


crates/cairo-lang-language-server/src/toolchain/scarb.rs line 130 at r1 (raw file):

Previously, Draggu (Piotr Figiela) wrote…

Why? You don't even check if it was successful or not.

Done.

Base automatically changed from spr/main/85c1c4f3 to main November 25, 2024 17:24
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.

3 participants