Skip to content
This repository has been archived by the owner on Feb 20, 2024. It is now read-only.

Allow configuring cairo-path from a toml file #29

Closed
kasperski95 opened this issue May 13, 2022 · 5 comments
Closed

Allow configuring cairo-path from a toml file #29

kasperski95 opened this issue May 13, 2022 · 5 comments
Labels
good first issue Good for newcomers

Comments

@kasperski95
Copy link

As a Protostar user, I want Cairo-LS read cairo-path from protostar.toml so that this VS Code extension show invalid imports correctly.

{
  "cairols.cairoPathConfigurations": [
    {
      "path": "./protostar.toml::protostar.shared_command_configs::cairo_path"
    },
    {
      "path": "./protostar.toml::protostar.build::cairo_path",
      "scopes": ["./src/*"]
    },
    {
      "path": "./protostar.toml::protostar.test::cairo_path",
      "scopes": ["./tests/*"]
    }
  ]
}
@ericglau
Copy link
Owner

@kasperski95 I think the following enhancements to Cairo-LS should work for this scenario. Let me know what you think.

  1. Look for protostar.toml in the project.
  2. Read the cairo_path settings that you mentioned.
  3. Add all of them to the Cairo search paths to be used for the compiler and for go-to-definitions etc.

Can you elaborate on what you mean by "scopes" above? How do you expect that to differ from just reading the cairo_path setting directly? e.g. what is the intended difference between paths 1 and 2/3 in your example?

@kasperski95
Copy link
Author

I think this extension should be independent of Protostar if possible. If we rename cairo_path to cairo_paths, this extension has to be updated. We may ask user for the permission to generate VS Code settings in protostar init.

Protostar ships with asserts that are meant to be used only in test files. The scope functionality would show an error in VS Code, if the user tried to import asserts in non-test files. However, this is a rare use case, therefore it might be not worth supporting it.

The structure of protostar.toml is a mapping of the CLI interface, hence the structure may look weird. To properly support protostar.toml the configuration would have to look like this:

{
  "cairols.cairoPathConfigurations": {
    "and": [
      {
        "or": [
          "./protostar.toml::protostar.build::cairo_path",
          "./protostar.toml::protostar.shared_command_configs::cairo_path"
        ]
      },
      {
        "or": [
          "./protostar.toml::protostar.test::cairo_path",
          "./protostar.toml::protostar.shared_command_configs::cairo_path"
        ]
      }
    ]
  }
}

I think the simplest support would cover most use cases. (The user probably won't use protostar.toml::protostar.test::cairo_path and protostar.toml::protostar.build::cairo_path.)

{
  "cairols.cairoPathConfigurations": [
    "./protostar.toml::protostar.shared_command_configs::cairo_path",
  ]
}

@ericglau ericglau added the good first issue Good for newcomers label May 31, 2022
@MaksymilianDemitraszek
Copy link
Contributor

MaksymilianDemitraszek commented Jun 6, 2022

I would like to work on this one as it's important for us as a protostar team to let users use protostar with cairo-ls.

In my opinion for now enough would be to let users to extra cairo-path by hand. Protostar's system is a bit more complicated and I don't think it's necessary to support it right now.

I would like to add an configuration filed

"cairols.cairoPath": {
	"scope": "resource",
	"type": "array",
	"default": [],
	"description": "Additional locations to search for cairo files"
},

and let user to enter extra cairo paths and pass it to the compiler.
We can work on the better integration with protostar later.
@ericglau WDYT?

@ericglau
Copy link
Owner

ericglau commented Jun 6, 2022

@MaksymilianDemitraszek That sounds good to me. Thanks for taking this on. Let me know if anything needs further discussion when you look at this.

Part of the effort would be to ensure the additional paths are used for both compilation and for go-to-definitions. The relevant parts of code should be

function getCompileCommand(settings: CairoLSSettings, tempFiles: TempFiles, textDocumentContents?: string): string {
and
async function initPackageSearchPaths(uri: string) {

@ericglau
Copy link
Owner

Resolved by #41 which implements this suggestion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants