Eliminate trivial underscore #147

2colours · 2024-12-01T13:32:20Z

Now, only the overly custom part of underscore-plus remains: this config reading and writing that should rather get a separate module/package.

The implementations themselves could be further improved as better Set operations land, mostly in Node v22.

This is the version that passes all the existing tests. The HTTP requests used to be handled by a callback that had three arguments: one for the errors if there are any, one for the response and one for the payload within the response. Now, the error has been turned into rejected promises (exceptions when awaited) and the payload has simply been dropped. It's extracted from the response via the .body attribute.

2colours · 2024-12-04T11:51:31Z

Gazing through the differences, this PR also seems to build upon #146 - that wasn't intentional but honestly, it's not "wrong enough" for me to try to go back. The content of that has been around for a long enough time, it's not going to change, and I think eventually it should get merged either way...

confused-Techie

I've reviewed your commit about removing outdated usage of underscore-plus.
Largely, I totally agree with the ways you've done so. Many of them being rather clever uses of built in features of the language.

But a recurring comment you'll find in my review is obscurity.

One obvious example:

// === Original Code
return _.compact(_.uniq(packageNames));

Here, even if you aren't familiar with underscore-plus you can quickly assume what it's doing based on the function name uniq and can hopefully reasonably guess at what compact is doing.

But if we take what it's changed to in this PR:

// === New Code
return Array.from(new Set(packageNames)).filter(Boolean);

While this code is generally faster by about 0.1ms we've lost the clues at what's going on. Now requiring someone to be familiar enough with JavaScript to know that a Set only allows unique values in each collection. Sure _.compact() => .filter() is more obvious, but you get what I'm saying.

So in many places I've added requests for just some simple comments explaining the purpose of some code. Since we want to try and keep the barrier to entry low and not require too much knowledge of how everything works in JavaScript. But this really only applies to the more esoteric uses. Otherwise considering this is my only major point to make, the rest of this looks pretty rad. Thanks! I'll try and get to the async commit soon

confused-Techie · 2024-12-05T05:06:43Z

src/ci.js

@@ -54,7 +53,11 @@ but cannot be used to install new packages or dependencies.\

    fs.makeTreeSync(this.atomDirectory);

-    const env = _.extend({}, process.env, {HOME: this.atomNodeDirectory, RUSTUP_HOME: config.getRustupHomeDirPath()});
+    const env = {
+      ...process.env, 


Just wanting to add, since this one made me a bit nervous, I've tested to confirm this behavior will match what underscore-plus does on it's particular version.

Ensuring that variables are overriden properly based on these news ones assigned. Smart use, and one of my favourites, of spread syntax.

I also wanted to be sure and specifically checked MDN - it's guaranteed that what comes later overrides earlier values. I really share the mixed feelings here: if everybody knew this, probably we wouldn't fear it but still, it doesn't seem trivial...

confused-Techie · 2024-12-05T05:16:26Z

src/command.js

@@ -58,7 +57,7 @@ class Command {
  sanitizePackageNames(packageNames) {
    packageNames ??= [];
    packageNames = packageNames.map(packageName => packageName.trim());
-    return _.compact(_.uniq(packageNames));
+    return Array.from(new Set(packageNames)).filter(Boolean);


Awesome work on this new methodology. I do want to point out that I did some light testing of this method compared to the original, and this new method does in fact shave off about 0.1ms on average compared to our usage of underscore-plus. So a small win, but still a win.

The only thing I'd like to request, removing the names of the previous functions compact and uniq does obscure the purpose of this code a bit. Maybe could we add a comment such as // Creates a unique valued truthy only list to ensure we know what this is supposed to do a year from now.

It would be hard to object against a banal comment for some logic that will probably never change but still, I do think that this concern is overreaching. I specifically chose .filter(Boolean) because I felt that was the clearest way to do it - way clearer than the misleading _.compact itself - and I also think it's common knowledge that you turn something to a set and then back in order to eliminate duplicates. Of all the ppm code, I definitely wouldn't be the most concerned about this...

confused-Techie · 2024-12-05T05:28:08Z

src/develop.js


-        return new Install().run(installOptions);
+      return new Install().run({...options});


Initially I was going to say we should hold off using spread syntax instead of clone() since it only deep copies initial object values. Leaving nested object values as a shallow copy.

This worried me, but then I tested and checked with underscore-plus, which does the exact same thing.

Considering the object we are working with is nested, I wonder how much we really need to be doing anything other than passing the value on here. Hard to say, and a discussion for another time. But this should be fine.
Although, like my previous comment, removing our function name obscures these purposes, maybe a comment like // Shallow copy top level properties to ensure someone doesn't haplessly just pass options like one could very easily assume

Again, almost sure I checked all functions and what they do, and also, I don't think it's all that obscure; specifically, I feel that merely passing a shallow copy without saying anything about why that's necessary is way more obscure than the spread syntax way of creating a shallow copy.

I totally agree that doing a shallow copy with no comment originally is more obscure, because when I wrote that even I was wondering why they ever needed to do that.

But I'm sure you checked the functions and what they do because I couldn't find any differing behavior in your changes which is awesome.

confused-Techie · 2024-12-05T05:35:50Z

src/star.js

@@ -77,7 +76,7 @@ Run \`ppm stars\` to see all your starred packages.\
        }
      }

-      return _.uniq(installedPackages);
+      return Array.from(new Set(installedPackages));


// Unique only array

confused-Techie · 2024-12-05T06:02:00Z

src/disable.js


          if (packageNames.length === 0) {
            return "Please specify a package to disable"; //errors as return values atm
          }

          const keyPath = '*.core.disabledPackages';
          const disabledPackages = _.valueForKeyPath(settings, keyPath) ?? [];
-          const result = _.union(disabledPackages, packageNames);
+          const result = Array.from(new Set([...disabledPackages, ...packageNames]));


I'm a bit worried about this code being rather esoteric looking, as in not being immediately understood in it's purpose at a glance.
Although it is significantly more performant than compared to both underscore's union() and the most verbose single liner I could think of:

const result = disabledPackages.concat(packageNames.filter(elem => !disabledPackages.includes(elem)));

With performance being roughly:

Underscore Plus: 0.167ms

Your Method: 0.007ms

My Method: 0.017ms

So considering this we should stick with what you've written, but I'd suggest again we add a comment to ensure the purpose of this is understood at a glance. Likely being able to use the comment from underscores union method: Produce an array that contains the union: each distinct element from all of the passed-in arrays or something similar

This is kind of a hack, after all; one that I probably found on SO. I don't know whose idea was to release Set in Javascript without adding set operations... they are coming with Node 22 iirc. I'm rather surprised I didn't outright add a TODO to replace these calls once the actual set operations land... it needs to be taken care of, one way or another.

confused-Techie

Just reviewed your changes to async for requests.
Overall it seems solid, I just had to ignore the issues me precursor PR is trying to resolve, as those aren't on you.

Other than a few mistakes of leaving in variables that don't seem to exist, this seems rather solid and should function fine.
Ideally, once merged I can refactor my PR and add back the changes I'm trying to get for better error logging.

confused-Techie · 2024-12-05T06:32:43Z

src/search.js

+        return packages;
+      }
+
+      const message = request.getErrorMessage(body, error);


The second argument error here is never defined. Is it intended to be collected in a missing try...catch statement?

I don't know, I didn't create getErrorMessage to begin with. 😅

Well I just mean, your changes removing the definition of the variable error. So I was asking if you intended to have it defined elsewhere, but if not then we will want to instead do request.getErrorMessage(body, null);

confused-Techie · 2024-12-05T06:38:31Z

src/unstar.js

+      const body = response.body ?? {};
+      if (response.statusCode !== 204) {
+        this.logFailure();
+        const message = request.getErrorMessage(body, error);


We will want to make sure to have the second parameter be null here, as error doesn't exist

confused-Techie · 2024-12-05T06:43:37Z

src/install.js

-          resolve(body);
-        });
+      const response = await request.get(requestSettings).catch(error => {
+        const message = request.getErrorMessage(body, error);


The first parameter here body doesn't exist, as we are only inside the catch() callback.

I feel this was "too clever". I rather don't want to change the logic here at the moment, I will revert to the original way of producing the error message here.

2colours · 2024-12-18T23:48:21Z

Here, even if you aren't familiar with underscore-plus you can quickly assume what it's doing based on the function name uniq and can hopefully reasonably guess at what compact is doing.

But if we take what it's changed to in this PR:
// === New Code
return Array.from(new Set(packageNames)).filter(Boolean);
While this code is generally faster by about 0.1ms we've lost the clues at what's going on. Now requiring someone to be familiar enough with JavaScript to know that a Set only allows unique values in each collection.

I suppose we must be very different people because I disagree categorically. Of course the strawman to beat would be compact which I find totally opaque but I cannot think of a scenario when one might think, using any tech stack, that something called a "set" would allow duplicates, and furthermore that taking a list of values, turning it into a set, and then turning it back into an array could serve any other purpose than making the elements unique. I strongly believe this is the reason there isn't any other built-in for this; it's obvious enough and better yet, portable across programming languages and technologies. I would agree that the uniqueness is achieved by unfolding something that seems more like an implementation detail - but that could actually be a plus in a lot of cases and I still don't think it does any significant damage.

savetheclocktower · 2024-12-19T00:10:40Z

I tend to fall more on @2colours’s side of this disagreement, but some short explanatory comments don't seem like they're a hardship here.

2colours · 2024-12-22T01:57:01Z

Alright, I addressed all the open issues. I think. Also, since the other PR is kinda spoiled and we are already addressing the request async topic here, I will just close that.

2colours added 2 commits December 1, 2024 03:29

Remove uses of underscore where it's obvious

358d44e

confused-Techie mentioned this pull request Dec 5, 2024

Asyncify request #146

Closed

confused-Techie requested changes Dec 5, 2024

View reviewed changes

Review related fixes

310756f

2colours requested a review from confused-Techie December 22, 2024 01:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate trivial underscore #147

Eliminate trivial underscore #147

2colours commented Dec 1, 2024

2colours commented Dec 4, 2024

confused-Techie left a comment

confused-Techie Dec 5, 2024

2colours Dec 18, 2024

confused-Techie Dec 5, 2024

2colours Dec 19, 2024

confused-Techie Dec 5, 2024

2colours Dec 19, 2024

confused-Techie Dec 20, 2024

confused-Techie Dec 5, 2024

confused-Techie Dec 5, 2024

2colours Dec 19, 2024

confused-Techie left a comment

confused-Techie Dec 5, 2024

2colours Dec 19, 2024

confused-Techie Dec 20, 2024

confused-Techie Dec 5, 2024

confused-Techie Dec 5, 2024

2colours Dec 22, 2024

2colours commented Dec 18, 2024

savetheclocktower commented Dec 19, 2024

2colours commented Dec 22, 2024


		return new Install().run(installOptions);
		return new Install().run({...options});

Eliminate trivial underscore #147

Are you sure you want to change the base?

Eliminate trivial underscore #147

Conversation

2colours commented Dec 1, 2024

2colours commented Dec 4, 2024

confused-Techie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

confused-Techie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2colours commented Dec 18, 2024

savetheclocktower commented Dec 19, 2024

2colours commented Dec 22, 2024