pietroppeter / nimib

nimib 🐳 - nim 👑 driven ⛵ publishing ✍
https://pietroppeter.github.io/nimib/
MIT License
183 stars 10 forks source link

Compile error when using custom Nimble environment variable #216

Closed sent44 closed 10 months ago

sent44 commented 1 year ago

https://github.com/pietroppeter/nimib/blob/dc890600a2274aaec8da71f5e16be084a7f15614/src/nimib/config.nim#L9-L11 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

HugoGranstrom commented 1 year ago

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 commented 1 year ago

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

HugoGranstrom commented 1 year ago

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

pietroppeter commented 1 year ago

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 commented 10 months ago

Fixed by #230