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

Compile error when using custom Nimble environment variable #216

Closed
sent44 opened this issue Aug 19, 2023 · 9 comments · May be fixed by #217
Closed

Compile error when using custom Nimble environment variable #216

sent44 opened this issue Aug 19, 2023 · 9 comments · May be fixed by #217

Comments

@sent44
Copy link
Contributor

sent44 commented Aug 19, 2023

let dumpedJson = execProcess("nimble dump --json", dir)
result = parseJson(dumpedJson)["version"].getStr()

When using nimble dump --json with custom Nimble environment variable, it will also output Info: Using the environment variable: ... at the front of json.
So when using that result string with parseJson(), it will error.
image

@HugoGranstrom
Copy link
Collaborator

If I understand this correctly, there is a Info: ... message that's in the json file? That sounds like a nimble bug, have you opened an issue in the nimble repo as well?

As for a temporary fix, I would like to avoid regex if possible. I would prefer if we could just trim off everything before the JSON starts instead. So basically remove everything until the first {. What do you think?

@sent44
Copy link
Contributor Author

sent44 commented Aug 19, 2023

How about mini lexical analysis? I don't trust crop until { as it might be false positive and the non --json make it very easy to do analysis.

About that bug, I do not open issue yet. I just feel lazy to do it.

@HugoGranstrom
Copy link
Collaborator

IMO, this is in principle a nimble bug, and we should not have to do a complex solution to this on our side. We should do the simplest solution possible. This is a general rule of coding that I wish I followed more myself. Implementing our own parser is too complex of a solution in my opinion, but I appreciate the initiative. :D

I don't trust crop until { as it might be false positive

Change the rule to the first { that is on its own line:

# remove this 
{
# keep this

I don't see how nimble would cause a false positive in this case. Even if a path contained a { it wouldn't affect it.

About that bug, I do not open issue yet. I just feel lazy to do it.

Someone should. That is the correct place to solve this issue. Let me know if you don't plan on opening the issue, and I'll do it myself :)

@sent44
Copy link
Contributor Author

sent44 commented Aug 19, 2023

While I say mini lexical analysis, I mean dumpText.find("version: ") 😅

@HugoGranstrom
Copy link
Collaborator

Oh, then that might actually be simpler 😯 If you make a PR that uses that approach (without using regex), I'm for approving it 😄

@sent44
Copy link
Contributor Author

sent44 commented Aug 19, 2023

I just thought of that after reply to you 😅
Currently outside so I can't do it now

@HugoGranstrom
Copy link
Collaborator

No hurry, take it when you have the time and energy 😁

@pietroppeter
Copy link
Owner

hi, agree that this is a nimble bug but instead of a workaround for our parsing I found a workaround looking at nimble code for display. There is indeed a --silent option to make sure that not message is displayed (see e.g. https://github.com/search?q=repo%3Anim-lang%2Fnimble%20SilentPriority&type=code). So the fix is to actually using this option, not changing our parsing

@pietroppeter
Copy link
Owner

Fixed by #230

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 a pull request may close this issue.

3 participants