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

Changed diagnostics to have full file paths #4024

Merged
merged 1 commit into from
Sep 7, 2023
Merged

Conversation

yuvalsw
Copy link
Contributor

@yuvalsw yuvalsw commented Sep 6, 2023

This change is Reviewable

@yuvalsw yuvalsw requested a review from orizi September 6, 2023 16:20
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 11 files at r1, all commit messages.
Reviewable status: 6 of 11 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)


crates/cairo-lang-filesystem/src/ids.rs line 115 at r1 (raw file):

    fn full_path(&self, db: &dyn FilesGroup) -> String {
        if let Some(parent) = self.parent {
            format!("{}[{}]", parent.full_path(db), self.name)

can you explain this path?

Code quote:

"{}[{}]

crates/cairo-lang-plugins/src/test.rs line 153 at r1 (raw file):

}

fn test_general_plugin(

is this related?

@yuvalsw yuvalsw force-pushed the yuval/full_path_errors branch from 6c50780 to c023d71 Compare September 7, 2023 08:24
Copy link
Contributor Author

@yuvalsw yuvalsw 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: 6 of 12 files reviewed, 2 unresolved discussions (waiting on @orizi)


crates/cairo-lang-filesystem/src/ids.rs line 115 at r1 (raw file):

Previously, orizi wrote…

can you explain this path?

I didn't want to have a.cairo/virtual_file/virtual_file2.
I hoped you'd suggest something else for the syntax of a virtual file path.


crates/cairo-lang-plugins/src/test.rs line 153 at r1 (raw file):

Previously, orizi wrote…

is this related?

Yes, I wanted to have a diagnostic with a path to a double-indirected file. See
"error: Double indirection diagnostic
--> test_src/lib.cairo[virt1][virt2]:1:1
"
in file "general".

@yuvalsw yuvalsw force-pushed the yuval/full_path_errors branch from c023d71 to 93e0164 Compare September 7, 2023 08:30
Copy link
Collaborator

@orizi orizi 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 11 files at r1, 2 of 2 files at r2.
Reviewable status: 9 of 12 files reviewed, 1 unresolved discussion (waiting on @yuvalsw)


crates/cairo-lang-filesystem/src/ids.rs line 115 at r1 (raw file):

Previously, yuvalsw wrote…

I didn't want to have a.cairo/virtual_file/virtual_file2.
I hoped you'd suggest something else for the syntax of a virtual file path.

add a todo to choose a more readable path.
and add to discussions.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r1.
Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @yuvalsw)

@yuvalsw yuvalsw force-pushed the yuval/full_path_errors branch from 93e0164 to 02fdfea Compare September 7, 2023 14:25
Copy link
Contributor Author

@yuvalsw yuvalsw 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: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-filesystem/src/ids.rs line 115 at r1 (raw file):

Previously, orizi wrote…

add a todo to choose a more readable path.
and add to discussions.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

@yuvalsw yuvalsw force-pushed the yuval/full_path_errors branch from 02fdfea to e8ab5ce Compare September 7, 2023 14:41
Copy link
Collaborator

@orizi orizi 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 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

@yuvalsw yuvalsw force-pushed the yuval/full_path_errors branch from e8ab5ce to 00bedd0 Compare September 7, 2023 14:50
Copy link
Collaborator

@orizi orizi left a 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 r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

@yuvalsw yuvalsw force-pushed the yuval/full_path_errors branch from 00bedd0 to c35ab62 Compare September 7, 2023 16:12
@yuvalsw yuvalsw force-pushed the yuval/full_path_errors branch from c35ab62 to 6529a7f Compare September 7, 2023 16:34
@yuvalsw yuvalsw enabled auto-merge September 7, 2023 16:38
@yuvalsw yuvalsw added this pull request to the merge queue Sep 7, 2023
Copy link
Collaborator

@orizi orizi left a 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 r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

Merged via the queue into main with commit ff89d35 Sep 7, 2023
@orizi orizi deleted the yuval/full_path_errors branch September 11, 2023 03: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.

2 participants