decaffeinate `src/apm-cli.js` + prepair file by file decaffeination #15

sertonix · 2022-11-03T18:21:18Z

This pull request is the first to decaffeinate the complete repository. It is required for further pull request because it moves .js files on build to the output folder.

package.json

confused-Techie · 2022-11-03T23:28:35Z

I thought I pinged you on Discord. But I do have a PR that decaffeinates the repo at #8.

Not sure if you want to keep going with this method, or it could be really helpful to commit directly to the PR of cleaning up the automatic files, since they are quite ugly and not the most readable.

Just wanting to make sure you're aware

sertonix · 2022-11-04T05:53:03Z

@confused-Techie
I looked at your pull request and first wanted to contribute to that but after a discussion on discord decaffeinating file by file seemed better.
https://discord.com/channels/992103415163396136/992110062992621598/1037755621962944592

GuilleW

Decaffeinate is ok for me

src/apm-cli.js

Co-authored-by: GuilleW <[email protected]>

DeeDeeG · 2022-11-06T21:02:34Z

It is admittedly hard to review all ~220-250 lines of code.

Can you describe what method you used to decaffeinate? What steps, what programs, so that we can reproduce your output. That would make it so we don't have to manually review every single line, and instead lean a lot heavier on trusting the tool that generated the JS output.

If the process you are using is extremely similar to #8, then we should indeed first decide whether to do all of this as one PR (#8) or one PR per file. As @confused-Techie mentioned.

(I am sorry to say the Discord link you posted does not take me to the relevant conversation, so I'm not sure exactly what was discussed about this before, who said what, etc.)

GuilleW · 2022-11-06T21:05:40Z

I review his code line by line.
compare .coffee and .js file.

sertonix · 2022-11-06T21:43:38Z

@DeeDeeG I used the notmal coffeescript compiler to convert the file and the rewrote parts with a more modern style. Reviewing line by line would be simpler than redoing this process. I will try to use the decaffeinating tool in the future to reduce the parts that are changed by me.

And I didn't used #8 because it was outdated and will be hard to merge beside other pull requests.

DeeDeeG · 2022-11-06T21:51:36Z

Thank you @sertonix. This is very helpful information. So, for example, I can run the coffeescript compiler and then do a diff with your file and review only the manual changes you made.

(This is much faster than having to read the whole coffeescript file and then the whole JS file and comparing them, function by function, line by line. I don't think any of us has the coffeescript code memorized by heart, so yes, we would have had to review all of the coffeescript just to review this, but now we can look at just the manual changes.)

GuilleW · 2022-11-06T21:55:06Z

I use decaffeinate-project online to convert, and refactor manually.

confused-Techie · 2022-11-06T23:51:43Z

I feel at this point as we talk about decaffeination tooling, I'd hate to repeat the past. But I also don't want to make anyone redo any work.

Luckily the tests in this PR seem to be passing, so likely not an issue here. But going forward please make sure to take a look at our documentation, as we have guidelines about decaffeination.

confused-Techie

Alright I've gone ahead and fully reviewed this file, while the decaffeination is absolutely fantastic, I do have some notes about the differences between the original source and JavaScript.

For some of these concerns I made a direct recommended change on the code itself. But theres two other more general concerns I wanted to share.

On Lines

61
123
163

Your code pattern looks something like let value = variable || defaultVariable;
While using the logical OR operator is fine and dandy, the original code has a pattern like let value = variable != null ? variable : defaultVariable;.

My concern is I don't know exactly what values we will be dealing with here. In general use it shouldn't make a difference, but really Logical OR operators check for truthy and falsy meanwhile using a Ternary Operator like original used is checking specifically for null. This could mean that we would find values like 0, "" or '' as valid values, meanwhile with these changes they would instead go to the default options.

I may suggest in these locations we instead revert to the original Ternary Operators being used.

Additionally in lines:

67
83

You use a logical not ! to check values, but again that checks for truthy/falsy values rather than strictly null like the original code.

Here I would again suggest to revert to checking directly for null like variable != null.

Personally I love the use of condensing these operations here, but just because we don't totally know what values we might find being returned in a function like this, I'd really recommend to again using the original checks. As time goes on and we find we are able, I'd love to increase the values we check for, but again only if we know then that it makes sense.

But as always, thanks a ton for contributing!

EDIT:

Thought I should comment on the changes requested below like ??, it's along the same lines, as those values used a Ternary Operator to check if the value was null and if so change it to the defaults. But since this is an assignment, we could go ahead and use the Nullish Coalescing Operator, to check if the left hand assignment is null, and return the default, otherwise do nothing. More of just matching the original constraints that this code had.

confused-Techie · 2022-11-15T05:45:33Z

src/apm-cli.js

+};
+
+function printVersions(args, callback) {
+  const apmVersion = require('../package.json').version || '';


Suggested change

const apmVersion = require('../package.json').version || '';

const apmVersion = require('../package.json').version ?? '';

confused-Techie · 2022-11-15T05:45:42Z

src/apm-cli.js

+
+function printVersions(args, callback) {
+  const apmVersion = require('../package.json').version || '';
+  const npmVersion = require('npm/package.json').version || '';


Suggested change

const npmVersion = require('npm/package.json').version || '';

const npmVersion = require('npm/package.json').version ?? '';

confused-Techie · 2022-11-15T05:45:53Z

src/apm-cli.js

+function printVersions(args, callback) {
+  const apmVersion = require('../package.json').version || '';
+  const npmVersion = require('npm/package.json').version || '';
+  const nodeVersion = process.versions.node || '';


Suggested change

const nodeVersion = process.versions.node || '';

const nodeVersion = process.versions.node ?? '';

confused-Techie · 2022-11-15T05:48:53Z

src/apm-cli.js

+  config.getResourcePath( resourcePath => {
+    const unknownVersion = 'unknown';
+    try {
+      const version = require(path.join(resourcePath, 'package.json')).version || unknownVersion;


Suggested change

const version = require(path.join(resourcePath, 'package.json')).version || unknownVersion;

const version = require(path.join(resourcePath, 'package.json')).version || 'unknown';

Could simply this function without declaring unknownVersion entirely.

confused-Techie · 2022-11-15T05:53:28Z

src/apm-cli.js

+    globalconfig: config.getGlobalConfigPath()
+  };
+  npm.load(npmOptions, () => {
+    let python = npm.config.get('python') || process.env.PYTHON;


Suggested change

let python = npm.config.get('python') || process.env.PYTHON;

let python = npm.config.get('python') != null ? npm.config.get('python') : process.env.PYTHON;

confused-Techie · 2022-11-15T05:54:20Z

src/apm-cli.js

+  npm.load(npmOptions, () => {
+    let python = npm.config.get('python') || process.env.PYTHON;
+    if (config.isWin32() && !python) {
+      let rootDir = process.env.SystemDrive || 'C:\\';


Suggested change

let rootDir = process.env.SystemDrive || 'C:\\';

let rootDir = process.env.SystemDrive != null ? process.env.SystemDrive : 'C:\\';

confused-Techie · 2022-11-15T06:02:10Z

src/apm-cli.js

+    spawned.on('close', code => {
+      let version;
+      if (code === 0) {
+        version = Buffer.concat(outputChunks).toString().split(' ')[1]?.trim();


Love the use of an optional chaining operator to trim() the version here.

My only concern is the original decaffienated code produces

[name, version] =Array.from(Buffer.concat(outputChunks).toString().split(' ')); version = version != null ? version.trim() : undefined;

So while the trim() is added perfectly the defaulting behavior to undefined isn't retained. I'm not sure if this is used later on or not, but maybe should be addressed? We could add a Nullish coalescing operator to the end to check for null or undefined

version = Buffer.concat(outputChunks).toString().split(' ')[1]?.trim() ?? undefined;

confused-Techie · 2022-11-15T06:03:33Z

src/apm-cli.js

+        if (_.isString(error)) {
+          message = error;
+        } else {
+          message = error.message || error;


Suggested change

message = error.message || error;

message = error?.message ?? error;

EDIT:

As for a quick explanation here, originally this line had the following pattern message = error.message != null ? error.message : error;
Which this checked if that specific method of error was null. Which is a handy way to check essentially if it's a valid method of the object, while also checking the return value. With your original change we run the method and check it's return only. Which if for whatever reason that's not a valid method, we can use the Optional Chaining ?. to check one if it's valid, then use the Nullish Coalescing operator to check the return value. Just thought this might protect against the unlikely event of message not being a valid method of error

mauricioszabo · 2022-11-15T19:39:04Z

src/apm-cli.js

+function parseOptions(args) {
+  if (!args) args = [];
+  const options = yargs(args).wrap(Math.min(100, yargs.terminalWidth()));
+  options.usage("\nPulsar Package Manager powered by https://pulsar-edit.com\n\n  Usage: apm <command>\n\n  where <command> is one of:\n  " + (wordwrap(4, 80)(Object.keys(commands).sort().join(', '))) + ".\n\n  Run `apm help <command>` to see the more details about a specific command.");


We can use the quote syntax here:

options.usage(` Usage: apm <command> where <command> is one of: ${wordwrap(4, 80)(Object.keys(commands).sort().join(', '))}. Run \`apm help <command>\` to see the more details about a specific command. `)

This will include all the spaces before the text starts. Therefor it would look like this and I think that is ugly.

options.usage(` Pulsar Package Manager powered by https://pulsar-edit.com Usage: apm <command> where <command> is one of: ${wordwrap(4, 80)(Object.keys(commands).sort().join(', '))}. Run \`apm help <command>\` to see the more details about a specific command.\ `);

The multiline boring problem 🙄
And using this style?

options.usage( "Pulsar Package Manager powered by https://pulsar-edit.com\n"+ "\n"+ "Usage: apm <command>\n"+ "....."

What do you think of this method?

options.usage([ "", "Pulsar Package Manager powered by https://pulsar-edit.com", "", " Usage: apm <command>", "", " where <command> is one of:", ` ${wordwrap(4, 80)(Object.keys(commands).sort().join(', '))}.`, "", " Run `apm help <command>` to see the more details about a specific command.", ].join("\n"));

The multiline boring problem 🙄 And using this style?

options.usage( "Pulsar Package Manager powered by https://pulsar-edit.com\n"+ "\n"+ "Usage: apm <command>\n"+ "....."

Oh, I didn't thought of that. Thanks

sertonix · 2022-11-15T20:57:47Z

This is now based on decaffeinate and not anymore on the default transpiler.

DeeDeeG · 2022-11-17T19:37:37Z

Sorry it has taken so long to get to properly reviewing this.

I tried to run npm test with this latest update. Only four of the tests run, and then the test harness silently quits somewhat uncleanly.

I don't think the code is functionally equivalent with the latest commits. (Mainly, the coffee compiler emits a JS file that is one big immediately-executed function, whereas I'm not sure that's true here?)

(Note on review etiquette: Please refrain from making huge changes (like switching to a different coffeescript compiler) while a review is already in progress. If you wanted to start over, I think it would make sense only to do that in a new PR, and let maintainers sort out the different approaches from there.

But generally being patient is the preferred thing. Fundamentally changing what is on offer for review, while review is ongoing, is mostly just going makes things more complicated. Please only make major changes on an existing pull request if the reviewers say they are okay with it.)

Sorry for lack of consensus before charging ahead with decaffeination in general, it seems like someone gave a casual go-ahead in Discord and a lot of work was started from that.

Taking a step back to look at the bigger picture for a moment: I think it would be a lot cleaner to just run the decaffeination scripts inherent to the repo (npm run build) and commit the result (at least on a "decaf" branch?), then allow any further clean-up work (and PRs) to target the "decaf base" branch. That way we are only reviewing the small manual changes, and we can maybe let go of the readability perfectionism in favor of a known 1:1 replacement, where any changes to that are expected to pass CI. Would make things much more doable in terms of difficulty of review, also.

(More rationale for that move: ppm technically already runs the machine-decaffeinated code in production. Committing the machine-decaffeinated files would not be a radical move, but a fairly conservative one.)

If other reviewers are okay with this, I can set up a branch as the base for any manual decaffeination changes. Just the machine-transpiled JS code with passing CI, so we can easily review any manual changes on top.
The other option would be to restore this PR branch to a working state, ideally based off the the coffee compiler, not decaffeinate, and patiently complete the existing review process.

(More thoughts: If we can't figure out consensus on how to clean up the machine-decaffeinated code, IMO we should just roll with the machine-decaffeinated code by default, to make sure we don't end up bike-shedding JS code style forever and ever. And this is an unfortunate type of first contribution, as it lends itself to bike-shedding and lots of small review comments, which would be require tremendous patience for a first-time contributor. Reviewers have no firm consensus to start with for how decaffeinated code should look like. It's really not great to have to all collaboratively figure this out on a first-time-contributor's first PR. So we should go into it understanding this is not an ideal situation, and it may take a lot of patience to get through it.)

Sorry for the essay-length comment, just trying to sort this whole thing out as it has already gotten somewhat messy, IMO.

I feel bad for the lack of timely review or clear expectations on when review will happen. But this decaf effort moved ahead rather quickly and without a complete plan for how the repo would need to be set up to accommodate the PRs, so that is what happens when things move without a detailed plan, we may enter uncharted territory and draw the map as we go. Running into snags wasn't so predictable. But it shouldn't be a surprise either, with how fast this went. I hope we can agree which way the path forward is and stick to it. Thanks for considering, everyone.

sertonix · 2022-11-17T21:52:17Z

@DeeDeeG
Sorry for the chaos with this and the decaf of the specs directory. The reason I chose and suggested this way was because the pull request from confused_techie was outdated and I feared that this would quickly happen again.

The functions wrapping the code produced from the normal coffeescript compiler is not needed. It ensures that no globals are assigned when directly loaded from html. This is not the case for this repo. (somebody correct me if this is not the real reason but I am 100% that it is not important for this)

For this and some other readability differences I strongly suggest using the decaffeinate tool.

But aside from this I am ok with a pull request that decafs the complete code without manual changes.

Since I have been working on other parts of pulsar lately I would really appreciate if somebody else could do that.

@GuilleW you said you could work on this. Sorry if I suggest things that were not really planned.
The new plan for you in short: Use a tool to decaf (maybe coffeescript compiler to really ensurre it is the same) everything and create a pull request with that. No manual changes this time.

sertonix · 2022-11-17T22:01:52Z

Also following this it would be nice is somebody could make the decaffeinate instructions in the "contributing during start" file more clear to prevent something like this in the future.

GuilleW · 2022-11-17T22:14:01Z

The functions wrapping the code produced from the normal coffeescript compiler is not needed. It ensures that no globals are assigned when directly loaded from html. This is not the case for this repo. (somebody correct me if this is not the real reason but I am 100% that it is not important for this)

Completely correct !

GuilleW · 2022-11-17T22:17:47Z

Waiting for a step-by-step instructions that admin agreed before working on.
Don't worry @sertonix , I'm ok to work on that part.
Stand by.

confused-Techie · 2022-11-18T00:22:11Z

@sertonix I do apologize for the confusion here. I'll go ahead and draft an update to our decaffeination policy, and would recommend taking a look for another PR. We all really do appreciate the efforts and contributions, just want to make sure we get this right.

sertonix added 4 commits November 3, 2022 18:01

decaffeinate src/apm-cli

4c915a2

spread array to multiple lines

0a9c8b8

use older syntax instead of ||=

fc44af0

allow decaffeinate file by file

950d6e4

This was referenced Nov 3, 2022

Decaffeinate src/apm #16

Closed

Decaffeinate src/auth #17

Closed

mauricioszabo reviewed Nov 3, 2022

View reviewed changes

package.json Show resolved Hide resolved

add comments to src/apm-cli.js

2d86bef

GuilleW suggested changes Nov 6, 2022

View reviewed changes

src/apm-cli.js Outdated Show resolved Hide resolved

src/apm-cli.js Show resolved Hide resolved

sertonix and others added 2 commits November 6, 2022 08:47

Update src/apm-cli.js

c33cedb

Co-authored-by: GuilleW <[email protected]>

Update src/apm-cli.js

f4666c8

Co-authored-by: GuilleW <[email protected]>

GuilleW approved these changes Nov 6, 2022

View reviewed changes

GuilleW mentioned this pull request Nov 6, 2022

Decaffeinate src/view #18

Closed

This was referenced Nov 7, 2022

Decaffeinate spec/spec-helper (Merge first: it's a dependency) #19

Closed

Decaffeinate spec/apm-cli-spec #20

Closed

confused-Techie requested changes Nov 15, 2022

View reviewed changes

mauricioszabo reviewed Nov 15, 2022

View reviewed changes

sertonix added 3 commits November 15, 2022 21:49

re decaf with decaffeinate

c3569bc

add more safety

bdfe2ae

better multiline string

efdd4d8

sertonix requested a review from confused-Techie November 15, 2022 20:58

sertonix closed this Nov 17, 2022

sertonix deleted the decaffeinate-apm-cli-js branch December 27, 2022 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

decaffeinate `src/apm-cli.js` + prepair file by file decaffeination #15

decaffeinate `src/apm-cli.js` + prepair file by file decaffeination #15

sertonix commented Nov 3, 2022

confused-Techie commented Nov 3, 2022

sertonix commented Nov 4, 2022

GuilleW left a comment

DeeDeeG commented Nov 6, 2022

GuilleW commented Nov 6, 2022

sertonix commented Nov 6, 2022 •

edited

Loading

DeeDeeG commented Nov 6, 2022 •

edited

Loading

GuilleW commented Nov 6, 2022

confused-Techie commented Nov 6, 2022

confused-Techie left a comment •

edited

Loading

confused-Techie Nov 15, 2022

confused-Techie Nov 15, 2022

confused-Techie Nov 15, 2022

confused-Techie Nov 15, 2022

confused-Techie Nov 15, 2022

confused-Techie Nov 15, 2022

confused-Techie Nov 15, 2022

confused-Techie Nov 15, 2022 •

edited

Loading

mauricioszabo Nov 15, 2022 •

edited

Loading

sertonix Nov 15, 2022

GuilleW Nov 15, 2022

sertonix Nov 15, 2022

sertonix Nov 15, 2022

sertonix commented Nov 15, 2022

DeeDeeG commented Nov 17, 2022 •

edited

Loading

sertonix commented Nov 17, 2022 •

edited

Loading

sertonix commented Nov 17, 2022

GuilleW commented Nov 17, 2022

GuilleW commented Nov 17, 2022

confused-Techie commented Nov 18, 2022

	const apmVersion = require('../package.json').version \|\| '';
	const apmVersion = require('../package.json').version ?? '';

	const npmVersion = require('npm/package.json').version \|\| '';
	const npmVersion = require('npm/package.json').version ?? '';

	const nodeVersion = process.versions.node \|\| '';
	const nodeVersion = process.versions.node ?? '';

	const version = require(path.join(resourcePath, 'package.json')).version \|\| unknownVersion;
	const version = require(path.join(resourcePath, 'package.json')).version \|\| 'unknown';

	let python = npm.config.get('python') \|\| process.env.PYTHON;
	let python = npm.config.get('python') != null ? npm.config.get('python') : process.env.PYTHON;

	let rootDir = process.env.SystemDrive \|\| 'C:\\';
	let rootDir = process.env.SystemDrive != null ? process.env.SystemDrive : 'C:\\';

	message = error.message \|\| error;
	message = error?.message ?? error;

decaffeinate src/apm-cli.js + prepair file by file decaffeination #15

decaffeinate src/apm-cli.js + prepair file by file decaffeination #15

Conversation

sertonix commented Nov 3, 2022

confused-Techie commented Nov 3, 2022

sertonix commented Nov 4, 2022

GuilleW left a comment

Choose a reason for hiding this comment

DeeDeeG commented Nov 6, 2022

GuilleW commented Nov 6, 2022

sertonix commented Nov 6, 2022 • edited Loading

DeeDeeG commented Nov 6, 2022 • edited Loading

GuilleW commented Nov 6, 2022

confused-Techie commented Nov 6, 2022

confused-Techie left a comment • edited Loading

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 Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

mauricioszabo Nov 15, 2022 • edited Loading

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

sertonix commented Nov 15, 2022

DeeDeeG commented Nov 17, 2022 • edited Loading

sertonix commented Nov 17, 2022 • edited Loading

sertonix commented Nov 17, 2022

GuilleW commented Nov 17, 2022

GuilleW commented Nov 17, 2022

confused-Techie commented Nov 18, 2022

decaffeinate `src/apm-cli.js` + prepair file by file decaffeination #15

decaffeinate `src/apm-cli.js` + prepair file by file decaffeination #15

sertonix commented Nov 6, 2022 •

edited

Loading

DeeDeeG commented Nov 6, 2022 •

edited

Loading

confused-Techie left a comment •

edited

Loading

confused-Techie Nov 15, 2022 •

edited

Loading

mauricioszabo Nov 15, 2022 •

edited

Loading

DeeDeeG commented Nov 17, 2022 •

edited

Loading

sertonix commented Nov 17, 2022 •

edited

Loading