npm / package-json

Programmatic API to update package.json
Other
68 stars 10 forks source link

refactor: add `normalizePackageMan` helper #100

Closed antongolub closed 5 months ago

antongolub commented 5 months ago

What / Why

Aligns normalization logic with directories.bin

See also: https://github.com/npm/normalize-package-data/blob/main/lib/fixer.js#L105

  fixManField: function (data) {
    if (!data.man) {
      return
    }
    if (typeof data.man === 'string') {
      data.man = [data.man]
    }
  },

References

CC @wraithgar

wraithgar commented 5 months ago

Looks like the tests aren't passing in Windows because of the classic / vs \ problem.

ETA: Look for the lines in the bin normalization code like: binKey.replace(/\\|:/g, '/') for how we deal with that there.

antongolub commented 5 months ago

The changes are separated now:

wraithgar commented 5 months ago

This will need a rebase because of #104 landing

antongolub commented 5 months ago

ref.replace(/\\|:/g, '/') is a common snippet, so I separated it into a function. If necessary, I can add another pull request to replace all similar statements with the helper.

wraithgar commented 5 months ago

If necessary, I can add another pull request to replace all similar statements with the helper.

Please do. I can land this PR now instead of waiting on that.

wraithgar commented 5 months ago

I wonder how much overlap there truly is between the bin and man normalization. I see that the bin normalization removes entries that reference nonexistent files while man doesn't.

If there's a 1:1 overlap would it be possible to pass bin or man to a shared function? If there are even one or two things that don't overlap that may be more work than it's worth but for things like this sharing code definitely helps, as your PRs this week have shown.

wraithgar commented 5 months ago

Also another small note about process. You don't need to @ the cli team for these. We have a codeowners file set up which automatically notifies us that a PR needs review when it's made.

wraithgar commented 5 months ago

Ah fiddlesticks I merged w/ an invalid prefix. I'll have to edit main now so heads up.

wraithgar commented 5 months ago

ok @antongolub the main branch is done being rewritten, if you git fetch now a PR made won't collide w/ the edited history.