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.

Sign up for GitHub

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

Eliminate trivial underscore #147

Merged
merged 3 commits into from
Dec 30, 2024
Merged

Eliminate trivial underscore #147

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28a3f0f
Changing own request.js to async interface
2colours Aug 31, 2024
358d44e
Remove uses of underscore where it's obvious
2colours Dec 1, 2024
310756f
Review related fixes
2colours Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions src/apm-cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const {spawn} = require('child_process');
const path = require('path');

const _ = require('underscore-plus');
const colors = require('colors');
const npm = require('npm');
const yargs = require('yargs');
Expand Down Expand Up @@ -215,12 +214,7 @@ module.exports = {
if (callbackCalled) { return; }
callbackCalled = true;
if (error != null) {
let message;
if (_.isString(error)) {
message = error;
} else {
message = error.message != null ? error.message : error;
}
const message = typeof error === 'string' ? error : error.message ?? error;

if (message === 'canceled') {
// A prompt was canceled so just log an empty line
Expand Down
7 changes: 5 additions & 2 deletions src/ci.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
const path = require('path');
const fs = require('./fs');
const yargs = require('yargs');
const _ = require('underscore-plus');

const config = require('./apm');
const Command = require('./command');
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

HOME: this.atomNodeDirectory,
RUSTUP_HOME: config.getRustupHomeDirPath()
};
this.addBuildEnvVars(env);

const installOptions = {env, streaming: options.argv.verbose};
Expand Down
3 changes: 1 addition & 2 deletions src/command.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@

const child_process = require('child_process');
const path = require('path');
const _ = require('underscore-plus');
const semver = require('semver');
const config = require('./apm');
const git = require('./git');
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

}

logSuccess() {
Expand Down
7 changes: 5 additions & 2 deletions src/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@

const path = require('path');
const _ = require('underscore-plus');
const yargs = require('yargs');
const apm = require('./apm');
const Command = require('./command');
Expand Down Expand Up @@ -37,7 +36,11 @@ Usage: ppm config set <key> <value>
let configArgs = ['--globalconfig', apm.getGlobalConfigPath(), '--userconfig', apm.getUserConfigPath(), 'config'];
configArgs = configArgs.concat(options.argv._);

const env = _.extend({}, process.env, {HOME: this.atomNodeDirectory, RUSTUP_HOME: apm.getRustupHomeDirPath()});
const env = {
...process.env,
HOME: this.atomNodeDirectory,
RUSTUP_HOME: apm.getRustupHomeDirPath()
};
const configOptions = {env};

return new Promise((resolve, _reject) =>
Expand Down
7 changes: 5 additions & 2 deletions src/dedupe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@

const path = require('path');

const _ = require('underscore-plus');
const yargs = require('yargs');

const config = require('./apm');
Expand Down Expand Up @@ -54,7 +53,11 @@ This command is experimental.\

fs.makeTreeSync(this.atomDirectory);

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

const dedupeOptions = {env};
Expand Down
54 changes: 24 additions & 30 deletions src/develop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
const fs = require('fs');
const path = require('path');

const _ = require('underscore-plus');
const yargs = require('yargs');

const config = require('./apm');
Expand Down Expand Up @@ -43,31 +42,25 @@ cmd-shift-o to run the package out of the newly cloned repository.\
return options.alias('h', 'help').describe('help', 'Print this usage message');
}

getRepositoryUrl(packageName) {
return new Promise((resolve, reject) => {
const requestSettings = {
url: `${config.getAtomPackagesUrl()}/${packageName}`,
json: true
};
return request.get(requestSettings, (error, response, body) => {
body ??= {};
if (error != null) {
return void reject(`Request for package information failed: ${error.message}`);
}

if (response.statusCode === 200) {
const repositoryUrl = body.repository.url;
if (repositoryUrl) {
return void resolve(repositoryUrl);
}

return void reject(`No repository URL found for package: ${packageName}`);
}
async getRepositoryUrl(packageName) {
const requestSettings = {
url: `${config.getAtomPackagesUrl()}/${packageName}`,
json: true
};
const response = await request.get(requestSettings).catch(error => Promise.reject(`Request for package information failed: ${error.message}`));
const body = response.body ?? {};

if (response.statusCode === 200) {
const repositoryUrl = body.repository.url;
if (repositoryUrl) {
return repositoryUrl;
}

throw `No repository URL found for package: ${packageName}`;
}

const message = request.getErrorMessage(body, error);
return void reject(`Request for package information failed: ${message}`);
});
});
const message = request.getErrorMessage(body, error);
throw `Request for package information failed: ${message}`;
}

async cloneRepository(repoUrl, packageDirectory, options) {
Expand All @@ -87,16 +80,17 @@ cmd-shift-o to run the package out of the newly cloned repository.\
}

installDependencies(packageDirectory, options) {
process.chdir(packageDirectory);
const installOptions = _.clone(options);
process.chdir(packageDirectory);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

linkPackage(packageDirectory, options) {
const linkOptions = _.clone(options);
const linkOptions = {
...options,
commandArgs: [packageDirectory, '--dev']
};

linkOptions.commandArgs = [packageDirectory, '--dev'];
return new Link().run(linkOptions);
}

Expand Down
10 changes: 5 additions & 5 deletions src/disable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,21 +59,21 @@ Disables the named package(s).\
try {
const installedPackages = await this.getInstalledPackages();
const installedPackageNames = Array.from(installedPackages).map((pkg) => pkg.name);
const uninstalledPackageNames = _.difference(packageNames, installedPackageNames);
if (uninstalledPackageNames.length > 0) {
console.log(`Not Installed:\n ${uninstalledPackageNames.join('\n ')}`);
const notInstalledPackageNames = packageNames.filter(elem => !installedPackageNames.includes(elem));
if (notInstalledPackageNames.length > 0) {
console.log(`Not Installed:\n ${notInstalledPackageNames.join('\n ')}`);
}

// only installed packages can be disabled
packageNames = _.difference(packageNames, uninstalledPackageNames);
packageNames = packageNames.filter(elem => installedPackageNames.includes(elem));

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]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_.setValueForKeyPath(settings, keyPath, result);

try {
Expand Down
8 changes: 4 additions & 4 deletions src/enable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,19 @@ Enables the named package(s).\
const keyPath = '*.core.disabledPackages';
const disabledPackages = _.valueForKeyPath(settings, keyPath) ?? [];

const errorPackages = _.difference(packageNames, disabledPackages);
const errorPackages = packageNames.filter(elem => !disabledPackages.includes(elem));
if (errorPackages.length > 0) {
console.log(`Not Disabled:\n ${errorPackages.join('\n ')}`);
}

// can't enable a package that isn't disabled
packageNames = _.difference(packageNames, errorPackages);
// can only enable a package that is disabled
packageNames = packageNames.filter(elem => disabledPackages.includes(elem));

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

const result = _.difference(disabledPackages, packageNames);
const result = disabledPackages.filter(elem => !packageNames.includes(elem));
_.setValueForKeyPath(settings, keyPath, result);

try {
Expand Down
34 changes: 18 additions & 16 deletions src/featured.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,29 +28,31 @@ List the Pulsar packages and themes that are currently featured.\
return options.boolean('json').describe('json', 'Output featured packages as JSON array');
}

getFeaturedPackagesByType(atomVersion, packageType) {
async getFeaturedPackagesByType(atomVersion, packageType) {

const requestSettings = {
url: `${config.getAtomApiUrl()}/${packageType}/featured`,
json: true
};
if (atomVersion) { requestSettings.qs = {engine: atomVersion}; }

return new Promise((resolve, reject) => void request.get(requestSettings, function(error, response, body) {
body ??= [];
if (error != null) {
return void reject(error);
}
if (response.statusCode === 200) {
let packages = body.filter(pack => (pack != null ? pack.releases : undefined) != null);
packages = packages.map(({readme, metadata, downloads, stargazers_count}) => _.extend({}, metadata, {readme, downloads, stargazers_count}));
packages = _.sortBy(packages, 'name');
return void resolve(packages);
}

const message = request.getErrorMessage(body, error);
reject(`Requesting packages failed: ${message}`);
}));
const response = await request.get(requestSettings);
const body = response.body ?? [];
if (response.statusCode === 200) {
let packages = body.filter(pack => pack?.releases != null);
packages = packages.map(({readme, metadata, downloads, stargazers_count}) => ({
...metadata,
readme,
downloads,
stargazers_count
}));

packages.sort((left, right) => left.name.localeCompare(right.name));
return packages;
}

const message = request.getErrorMessage(body, error);
throw `Requesting packages failed: ${message}`;
}

async getAllFeaturedPackages(atomVersion) {
Expand Down
60 changes: 34 additions & 26 deletions src/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
const assert = require('assert');
const path = require('path');

const _ = require('underscore-plus');
const async = require('async');
const CSON = require('season');
const yargs = require('yargs');
Expand Down Expand Up @@ -82,7 +81,11 @@ package names to install with optional versions using the

fs.makeTreeSync(this.atomDirectory);

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

const installOptions = {env};
Expand Down Expand Up @@ -194,7 +197,11 @@ Run ppm -v after installing Git to see what version has been detected.\

fs.makeTreeSync(this.atomDirectory);

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

const installOptions = {env};
Expand All @@ -209,32 +216,26 @@ Run ppm -v after installing Git to see what version has been detected.\
// packageName - The string name of the package to request.
//
// return value - A Promise that rejects with an appropriate error or resolves to the response body
requestPackage(packageName) {
async requestPackage(packageName) {
const requestSettings = {
url: `${config.getAtomPackagesUrl()}/${packageName}`,
json: true,
retries: 4
};
return new Promise((resolve, reject) => {
request.get(requestSettings, (error, response, body) => {
let message;
body ??= {};
if (error != null) {
message = `Request for package information failed: ${error.message}`;
if (error.status) { message += ` (${error.status})`; }
return void reject(message);
}
if (response.statusCode !== 200) {
message = request.getErrorMessage(body, error);
return void reject(`Request for package information failed: ${message}`);
}
if (!body.releases.latest) {
return void reject(`No releases available for ${packageName}`);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw `Request for package information failed: ${message}`;
});
const body = response.body ?? {};
if (response.statusCode !== 200) {
const message = request.getErrorMessage(body, null);
throw `Request for package information failed: ${message}`;
}
if (!body.releases.latest) {
throw `No releases available for ${packageName}`;
}

return body;
}

// Is the package at the specified version already installed?
Expand Down Expand Up @@ -369,7 +370,10 @@ Run ppm -v after installing Git to see what version has been detected.\
//
// return value - A Promise that rejects with an error or resolves without a value
async installPackageDependencies(options) {
options = _.extend({}, options, {installGlobally: false});
options = {
...options,
installGlobally: false
};
const commands = [];
const object = this.getPackageDependencies(options.cwd);
for (let name in object) {
Expand Down Expand Up @@ -452,7 +456,11 @@ Run ppm -v after installing Git to see what version has been detected.\

fs.makeTreeSync(this.atomDirectory);

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

const buildOptions = {env};
Expand Down Expand Up @@ -723,7 +731,7 @@ with Pulsar will be used.\
const iteratee = async fn => await fn();
try {
let installedPackagesInfo = await async.mapSeries(commands, iteratee);
installedPackagesInfo = _.compact(installedPackagesInfo);
installedPackagesInfo = installedPackagesInfo.filter(Boolean);
installedPackagesInfo = installedPackagesInfo.filter((item, idx) => packageNames[idx] !== ".");
if (options.argv.json) { console.log(JSON.stringify(installedPackagesInfo, null, " ")); }
} catch (error) {
Expand Down
Loading