Chai: Move pathval into its own module

[keithamus]

We're currently not using pathval and the code on the pathval github repo is quite old. It would be good to make the effort to move the pathval code which resided in chai (here, here and here) into the pathval module.

Pathval module itself should also be updated to the same standards type-detect and check-error are - so using the same build, lint, code coverage, and browser testing tools.

[lucasfcosta]

Well, if there isn't anyone doing this I'd be happy to help with this issue. Since I've done the check-error repo I think I'm pretty familiar with the whole process.

Would you mind if I do so, @keithamus and @meeber?

[keithamus]

I'm happy if @meeber's happy 😄

[meeber]

Have at it!

[lucasfcosta]

Thanks guys, I'm going to have a lot of fun with this on this weekend :smile:

[lucasfcosta]

Hello folks, I'd like to update you on the status of this issue and also ask some questions.
Currently I've moved the whole code to the pathval repo and the project structure is also done. All that's left is the tests to get 100% coverage, and the rest depends on your answers to the questions below:

1. Should I refactor pathval? I think it would be a nice thing to do, but I'm not sure this fits into the scope since it can generate new bugs or unexpected behavior. Should I do it or not?
2. I noticed that here we don't use pathval'sset method, but it seems like an important method for pathval as a standalone method, so I thought it would be nice to keep it, what do you think about it?
3. Can I tweak .eslintrc rules a little bit? Some rules are too strict, such as making initializing a variable mandatory (even though I want it to really be undefined) and not allowing only if/else branches inside an else block. Currently there is no elegant way to make pathval work with these rules, so instead of using eslint-disable-line I would prefer disabling these rules at all.

Well, I think that's all, thanks for you attention :smile:

[keithamus]

1. What kind of refactor do you have in mind? One that impacts tests or not? Bear in mind we'll be releasing within chai as 4.0 so if it breaks compat its not the biggest problem.
2. Unequivocally yes - keep .set - part of the reason of splitting these libs out is to make them independently useful.
3. Let's have a discussion about each of the rules that you feel are too strict individually. You mention the init-declarations and no-lonely-if so let me give my justifications for them:
   
   
   
   • init-declarations: This rule is 2 fold. For one, forcing you to init declarations forces you to think about how you're using the variable, what type it will start out life as, and most importantly what the default value will be. In many cases you might, say, declare it - do some work in if statements to assign it and ultimately return that value. IMO this is a slightly bad practice because you usually dont want to return undefined, but you're making it an explicit path by not assigning the variable to begin with. The second part of that is that in the cases where you _really really really_ want it to have an "uninitialised value" - set it to null, that's what null is there for, null also gives the consumers of your API a result which has a clear and explicit intention. null always means return null (or return someValueThatIsNull), undefined means that either the function exited without returning, it returned nothing (return), return undefined (explicit), return someValueThatNeverGotAssigned, return someValueThatWasExplicityAssignedToUndefined, return void... the list goes on. The point being, undefined is a "bad part" of JavaScript.
   
   
   • no-lonely-if: this usually comes down to readability, and it is an attempt to avoid gratuitously nested blocks. If you have an if inside an else... that else is probably an else if. The old adage "code is read more often that it is written" rings true here - nesting logic means readers have to deploy a finite state machine in their head to keep track of the logic, and that is never a good idea. An aside; in my own code I generally try to avoid using else altogether because reading code that just has a set of ifs makes things much more readable - ifs are context free, whereas else says "all of the context a dozen lines above me, yeah - not that".
   
   

I've been meaning to write a set of proper documents about the justification for each rule in that strict config - but I stand by each one of them as valid rules. I want to make sure we are all able to write awesome code though - so if you feel strongly about removing them, let's look into that.

[lucasfcosta]

@keithamus Thanks for the explanation! Well, since I'll rewrite the code I think those rules won't be much of a problem, however, thanks for the detailed explanation on the reason for them.
IMO the no-lonely-if really makes sense and I think you're completely right.
I was worried about init-declarations because of compatibility, I would prefer returning null too, but I was worried if we should keep the exact same behavior pathval had before or not (and by pathval I mean the parts of it inside Chai's core too), so returning null definitely solves the problem with this rule too.

Thanks again, Keith :smile:

[meeber]

Speaking of linting... Chai core needs it badly. I'd like to make it a priority after v4 is released.

Speaking of v4 release... How do we want to approach merging the 4.x.x branch? I imagine it'll require extensive conflict-resolution jiu jitsu.

[keithamus]

Speaking of linting... Chai core needs it badly

Yup. Part of the point of splitting out the utils into separate modules is so we can tackle standardising things like code coverage and linting piecemeal. Hopefully by chai 5.0 or 6.0 the chaijs/chai repo will be nothing more than a package.json 😄

Speaking of v4 release... How do we want to approach merging the 4.x.x branch

Once we're ready with this module, deep-eql and maybe loupe - someone will need to sit down and tackle the 4.x.x branch. I occasionally rebase it when I have some downtime, so it shouldn't be the worst thing by now, but I'm sure it has plenty of conflicts still 😞

[lucasfcosta]

Well, I'm available to help whenever you guys want to merge 4.x.x. Maybe scheduling some time to do it together would be cool since we all have touched different parts of the code.

[meeber]

@lucasfcosta Correct me if I'm wrong, but the hard part is done (moving pathval to separate module), so all that's left is cleaning up some pathval code in Chai?

[lucasfcosta]

@meeber Yes! All that's left is clean up the old pathval code in Chai's core and then making it use the external module.
I plan on doing it this weekend.

[lucasfcosta]

Hello everyone!
So, the only thing left before I can start cleaning up the old pathval code in Chai's core is the publication of the new pathval module to NPM.
Let me know if there's any way for me to do this or if you need any help.

Thanks in advance 😄

[keithamus]

Yes I'm going to put some work into this now. Maybe I'll just re-assign this to me actually.

[lucasfcosta]

Thanks @keithamus, feel free to re-assign me whenever you are done, there's no need to rush 😄

[keithamus]

@lucasfcosta hoped to carve out time to tackle this but I think I'll leave to you and focus on deep-eql 😅

[lucasfcosta]

@keithamus no problem buddy! :smile:
So, where can I get the saucelabs tokens to put into the travis.yml file?
I think that is the only thing left for us to publish that.
You can read more about it at chaijs/pathval#23.

[keithamus]

Oh right, I actually have to do that LOL. Okay, I'll reassign to me and get this done tonight.

[keithamus]

I'm going to close this, we merged #830 - @lucasfcosta if there is stuff left to do here could you please re-open.

[lucasfcosta]

For now that's all, thanks @keithamus!