pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 13 forks source link

Decaffeinate remaining spec files from list-spec on #78

Closed 2colours closed 1 year ago

2colours commented 1 year ago

This PR is meant to supersede https://github.com/pulsar-edit/ppm/pull/35.

Honestly, I don't think there is an easy way around it - I took all the commits from @GuilleW, readded the semicolons and compared the files to the decaf branch state of things, much like a review. Sometimes new code needed to be added, sometimes nodeVersion needs to be read from config, sometimes the function signatures could be improved.

I don't think there are more grandiose changes than previously but the amount of them is relatively big compared to the single-file PRs.

confused-Techie commented 1 year ago

@2colours Glad to see we are almost at the end of the decaf work on spec files!

Since I'm assuming this is the last one on the list. But want to mention, specs are failing here, so it seems something's not happy with this decaf

2colours commented 1 year ago

@confused-Techie Yes, well, that was to be expected, this time around I only had a vanilla Meld to aid me so even a sneaky syntax error is imaginable. Just please, if you know where the error is, do tell me, I don't even know what "the specs are failing" means in practice.

confused-Techie commented 1 year ago

So in programming, specs or tests refer to tests run against the code. Oftentimes on GitHub, tests or specs will be run automatically during PRs. like you'll see on this PR we have several tests/specs that run on every PR you've made. And here they are all failing. If you click on the details link of the test run it'll show you a CLI output of the test that will tell you exactly where the error is, or whatever output the testing framework shows for test runs.

If we click on the details of yours it does in fact show that there was an unexpected ) on line 165 of the list-spec.js file. So presumably we could take a look at that line to see what was going on. (Which I've done, but didn't seem to fix any of the tests. Meaning the JavaScript code you've submitted is invalid somewhere else.)

Generally, as PR's are a way of saying here is my code that I think would be good to submit, it's up to the one making the PR to investigate what's gone wrong. Because I didn't know what had gone wrong, since I just saw the failing status. Don't worry I'll check it out and get it fixed.

2colours commented 1 year ago

Generally, as PR's are a way of saying here is my code that I think would be good to submit, it's up to the one making the PR to investigate what's gone wrong.

In this particular case, though, since I'm just a mere "garbage collector" who doesn't even use this software, let alone actively develop it, I think it would be kind of unfair to expect me to set up the whole environment for myself to test things out. The whole premise is that I can do something so somebody else doesn't have to, it doesn't mean that I will "double down" on it, should something go off, that's above my paycheck.

Of course, it would be different if there were a ready-made environment (a Docker container for example) that somebody can just quickly load and use without much hassle; or if the named CI builds didn't tend to fail even for pull requests that were apparently right.

Anyway, now that I'm back home, I was going to check at least the syntax either way, and there isn't necessarily much more that should be done, hopefully.

DeeDeeG commented 1 year ago

Thanks for fixing the tests. They are passing again.

I will hope to make some progress reviewing these PRs soon. (This one is a big one, of course, but it can be reviewed file by file, and individual files can be marked as "seen" in the "changed files" tab, to make it easier for reviewers to do part of the review and come back to it in another session, at least I think it saves these markings across sessions? Worth trying.)