Recognize crate plugin dependencies #208
Conversation
integraledelebesgue
requested review from
mkaput and
piotmag769
and removed request for
a team
integraledelebesgue
force-pushed
the
spr/main/fa144cb1
branch
from
eb2d8ce to
659c827
Compare
integraledelebesgue
force-pushed
the
spr/main/a1e078b6
branch
from
58c95c5 to
f6fa869
Compare
integraledelebesgue
force-pushed
the
spr/main/fa144cb1
branch
2 times, most recently
from
a1a6506 to
639396f
Compare
integraledelebesgue
force-pushed
the
spr/main/fa144cb1
branch
2 times, most recently
from
98cc352 to
321bd50
Compare
|
Please don't merge until we have the metadata changes on main |
src/project/builtin_plugins.rs
Outdated
|
|
||
| let package_name = &plugin_metadata.package.repr; | ||
|
|
||
| if package_name.contains("assert_macros") { |
There was a problem hiding this comment.
Why .contains() instead of regular ==? We can make true here on package like assert_macros_but_better, don't we?
There was a problem hiding this comment.
package_name stores a serialized name, containing the version and source. For assert_macros, it is assert_macros 2.9.2 (std). That's why this method checks whether the plugin is built in and then whether its name .contains the important part. Alternatively, we can tokenize those names and use == as you suggested, or use regular expression matching.
There was a problem hiding this comment.
That's fine, but please comment on this at least.
There was a problem hiding this comment.
Sure, I added an explanation
There was a problem hiding this comment.
Now that I think about it maybe regex is indeed better xD
There was a problem hiding this comment.
// The package discriminator has form: "<name> <version> (<source>)".
So maybe .starts_with("<name> ") (note space after name)?
You will not catch it in the source and it can be arbitrary link.
integraledelebesgue
force-pushed
the
spr/main/fa144cb1
branch
2 times, most recently
from
c43464a to
c161fce
Compare
| let package_discriminator = &plugin_metadata.package.repr; | ||
|
|
||
| // Check if the discriminator refers to a known built-in plugin package. | ||
| if package_discriminator.contains("assert_macros") { |
There was a problem hiding this comment.
do at least this
| if package_discriminator.contains("assert_macros") { | |
| if package_discriminator.starts_with("assert_macros ") { |
this will be immune to packages having this keyword in their source part
but I think to be entirely sure, you should not inspect the ID, but look for package metadata instead and do == equality over package name + possibly whitelist source
integraledelebesgue
force-pushed
the
spr/main/fa144cb1
branch
from
c161fce to
48c6c86
Compare
integraledelebesgue
force-pushed
the
spr/main/a1e078b6
branch
from
f6fa869 to
cf91f9b
Compare
| @@ -58,7 +58,7 @@ lsp-server = "0.7.8" | |||
| lsp-types = "=0.95.0" | |||
| memchr = "2.7.4" | |||
| salsa = { package = "rust-analyzer-salsa", version = "0.17.0-pre.6" } | |||
| scarb-metadata = "1.13" | |||
| scarb-metadata = { git = "https://github.com/software-mansion/scarb", branch = "spr/main/ac416d0e" } # TODO: Change back to crates-io as soon as changes from that branch are released there | |||
There was a problem hiding this comment.
Blocking until these changes are indeed released. I don't want to fall into trap when Cairo 2.10 is released and we don't have this fixed yet
commit-id:a1e078b6
commit-id:fa144cb1
integraledelebesgue
force-pushed
the
spr/main/a1e078b6
branch
from
cf91f9b to
61468b9
Compare
integraledelebesgue
force-pushed
the
spr/main/fa144cb1
branch
from
48c6c86 to
c90354f
Compare
Stack: