Skip to content

Conversation

@VoltrexKeyva
Copy link
Member

  • Rewrite to ESM.
  • Refactor the JavaScript files.
  • Update dependencies and developer dependencies to their latest versions.
  • Format files with Prettier.
  • Add package-lock.json to .gitignore.
  • Add missing backtick in README.md.

@VoltrexKeyva
Copy link
Member Author

VoltrexKeyva commented Apr 1, 2023

Pinging @rvagg and @Trott since this repository isn't active.

Note: the npm package of this library should also be updated because Snyk caught a vulnerability in the nodejs/node-core-utils that traces back to this library, the npm package of this library hasn't been updated in 6 years.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

seems fine to me, but involving my personal code style preference I wouldn't mind seeing:

  • Apply standard to this, and in particular ditch the semicolons, they're poking my eyes
  • Single-line ifs are gross (don't look at my old code pls) and hard to parse, these days I'd much prefer explicit bracing of every if and else
  • Some random blank lines that don't seem to have a pattern, e.g. line 42. I like spacing out code, as long as there's a rhyme to is. Otherwise random blank lines are weird.

Also, thanks for giving it some love!

- Rewrite to ESM.
- Refactor the JavaScript files.
- Update dependencies and developer dependencies to their latest
versions.
- Format files with Prettier.
- Add `package-lock.json` to `.gitignore`.
- Add missing backtick in `README.md`.
}
commit.reviewers.push({ name: m[1], email: m[2] })
} else if (m = line.match(/^\s+PR(?:[- ]?URL)?:?\s*(.+)\s*$/)) {
} else if ((m = line.match(/^\s+PR(?:[- ]?URL)?:?\s*(.+)\s*$/)) !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

interesting, did standard complain about this or are you just opting to be explicit (not objecting, just intested)

Copy link
Member Author

Choose a reason for hiding this comment

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

The standard tool didn't complain about it, I'm just making it explicit.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for indulging my preferences!

@rvagg rvagg mentioned this pull request Apr 4, 2023
@rvagg
Copy link
Member

rvagg commented Apr 4, 2023

See #8, I've pulled this commit in to there and would like to merge over there than here so we can get an auto-publish flow working.

@rvagg
Copy link
Member

rvagg commented Apr 4, 2023

@rvagg rvagg closed this Apr 4, 2023
@VoltrexKeyva VoltrexKeyva deleted the refact-and-deps branch April 5, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants