truncate_with_package_name sometimes grabs the wrong name

[fracmak]

If the repo "name" isn't the first name in the package.json file, it'll print out the wrong name.

Example:

https://github.com/gajus/babel-plugin-react-css-modules/blob/master/package.json

spits out "Gajus Kuizinas" as the project name instead of "babel-plugin-react-css-modules"

[bhilburn]

Not knowing much about package.json files myself, I'm not sure of the best way to handle this.

Any thoughts from our JS devs?

[V1rgul]

It was done this way to avoid external dependencies or firing up an interpreter.
It could be done with a regular expression, but it will either:

• only support a maximum bracket depth
• require PCRE (see: http://zsh.sourceforge.net/Doc/Release/Conditional-Expressions.html)

Using a complex RegExp supporting all cases will surely be cpu intensive (~2ms for bracket depth=1 and the exemple file).
Also noting that Regular Expressions are bad for code simplicity.

So at this point I'll recommend using, if present: jq, node, python or the current method, to decode the json.

[bhilburn]

Thanks for the expert opinion, @V1rgul!

Based on this, what, if any, changes would you recommend to our existing segment code?

[V1rgul]

I think we should use jq.
Using nodejs as a fallback isn't an option:

time (repeat   10 { node -e 'console.log(require(process.argv[1]).name);' ./package.json >> /dev/null })
0,45s user 0,03s system 96% cpu 0,495 total
time (repeat   10 { jq '.name' package.json > /dev/null })  
0,01s user 0,01s system 51% cpu 0,039 total
time (repeat 1000 { jq '.name' package.json > /dev/null })  
0,03s user 0,60s system 36% cpu 1,731 total

So the question stands with, do we still use the current method as a fallback ?

• Yes : Users might not see the need to install jq, and have invalid truncation
• No : Users need to install jq, where they possibly can't

[fracmak]

This is very much an edge case, this is the first repo out of hundreds that I've seen done this way. It might not be with fixing if the performance is way worse. I agree node.js is too slow

[bhilburn]

Thanks for the testing, @V1rgul. I agree with you @fracmak that `node.js' is clearly too slow.

I'm always very hesitant to add additional dependencies / requirements for a working P9K installation, but it would be good to somehow inform users that installing jq will improve the performance of this segment.

Here's my suggestion: When P9K initializes, use the segment_in_use utility function to test for whether or not this segment is on. If it is, print a one-time warning that recommends the use of jq.

Thoughts?

[fracmak]

I ran some tests locally and jq is definitely faster than the current awk/grep combo

time (repeat 1000 { jq '.name' package.json > /dev/null })                                                                                     
3.31s user 1.21s system 77% cpu 5.820 total
time (repeat 1000 { cat "./package.json" 2> /dev/null | grep -m 1 "\"name\"" | awk -F ':' '{print $2}' | awk -F '"' '{print $2}' }) > /dev/null
4.86s user 4.15s system 190% cpu 4.729 total

[dritter]

Just a quick idea: Why not trying more than one method to extract the name? I would do it like that (pseudo code):

local pkgFile="package.json"
local packageName=$(jq '.name' ${pkgFile} 2> /dev/null || node -e 'console.log(require(process.argv[1]).name);' ./${pkgFile} 2>/dev/null || cat "./${pkgFile}" 2> /dev/null | grep -m 1 "\"name\"" | awk -F ':' '{print $2}' | awk -F '"' '{print $2}' 2>/dev/null)

It would try the jq first, then fail gracefully, try node, and at last do it with grep and awk. That way jq won't be a hard dependency.

@fracmak Awesome testing! 👍

[bhilburn]

Actually, my understanding is that @dritter and @V1rgul are suggesting the same thing. @V1rgul's point, I think, was that by failing gracefully, it enables users to not have to install jq, which could lead to them seeing improper truncation.

I do want to fail gracefully because I don't want to add a dependency, but I also want to make users aware that installing jq will give them a better segment.

[bhilburn]

@fracmak - I'd like to move ahead with getting this merged. Let's document that jq is the proper solution in the README, but fall back to the current regex to avoid mandating additional dependencies. That does mean it's possible users will end up with invalid truncation, but I think that's the right compromise.

@fracmak @V1rgul - What needs to happen on this branch to finalize it for merge?

[V1rgul]

The exemple dritter commented should be ok, it just needs to be integrated into the source

[dritter]

Hi all!

I created PR #411 for this. Could you confirm that the fix works as expected? That would be nice. 👍

[bhilburn]

Whoa, awesome work, @dritter!

@fracmak @V1rgul - Can you guys give @dritter's branch a try and see if it works for you?

[bhilburn]

Merged #411 into next.