purescript-contrib / governance

Guidelines and resources for the PureScript Contributors organization
15 stars 3 forks source link

Fix CI failures because of unused names or importing transitive dependencies #43

Closed thomashoneyman closed 3 years ago

thomashoneyman commented 3 years ago

A number of our libraries are failing in CI for one of two reasons:

  1. They directly import from a package that isn't explicitly listed as a dependency in their spago.dhall file
  2. They contain unused names in their code (ie. an unused function, or an unused argument, or a destructured value that is never used...)

They need to be fixed by taking actions depending on whether they're failing due to the unused names or the transitive dependencies. If you would like to be included in the CHANGELOG you are free to add an entry under the "Other improvements" section (for example: Ensure all imported packages are in the spago.dhall file (#000 by @___)).

Don't bother updating the bower.json files -- I can take care of this later on!

To fix a library that imports from a transitive dependency:

  1. Follow the instructions Spago includes about installing missing libraries -- this can basically be copy/pasted into a call to spago install. For example, if Spago reports that a library imports from prelude but doesn't list it as a dependency, then you can fix the issue with spago install prelude.
  2. If there are warnings about unused dependencies, then remove the libraries that Spago has identified as unused from the package's spago.dhall file. This has to be done manually -- there is no uninstall command.

To fix a library that has an UnusedName error from the compiler:

  1. If the error is reported on a top-level declaration that is unused and not exported, then remove the declaration. (If it should be exported instead, then it will be caught in code review.)
  2. If the error is about a declaration that is unused, then remove the declaration.
  3. If the error results from destructuring (for example, destructuring a record { a, b } but one or more of the names is unused), then remove the unused names.

When you fix one of these libraries, please include a link to this issue in the pull request so that it automatically shows up on this issue. These libraries need to be fixed:

pete-murphy commented 3 years ago

Seems like there were a few people interested in working on this, not sure what the plan is to coordinate, but FWIW, I'll try my hand at fixing aff (unless anyone's already claimed it 😄) and then see how it goes from there.

ntwilson commented 3 years ago

now is fixed in an earlier PR of mine: https://github.com/purescript-contrib/purescript-now/pull/20/files I'd be willing to split it into two if that's worthwhile

pete-murphy commented 3 years ago

I'll try to get to affjax, aff-coroutines & aff-bus next

ntwilson commented 3 years ago

I'd love to grab the argonaut family of packages.

thomashoneyman commented 3 years ago

Coordination essentially comes down to checking if there is already a PR or if the library is already checked off 😬 I'm open to ideas for coordination! I'll review and merge things as quickly as I can.

pete-murphy commented 3 years ago

Ah whoops, I didn't think to update the bower.json in any of my PRs, will get to that

ntwilson commented 3 years ago

Is there a good way to check that bower.json got updated correctly (right version numbers, and the correct split between "dependencies" and "devDependencies")?

thomashoneyman commented 3 years ago

~The bower.json files don't have to be updated, though I definitely appreciate it. It's a bit annoying so feel free to skip it. You can generate the bower.json files from the Spago configurations. No developer in the organization uses Bower, so there's no need to preserve devDependencies.~

On second thought -- don't worry about the bower.json files. For libraries that have already had this done, it's all good, but due to quirks with the library publishing process right now it's going to be more trouble than it's worth for everyone to worry about it.

If you want to be included in the CHANGELOG then you can add this change under the "Other improvements" section, but you don't need to do this if you don't feel like it.

artemisSystem commented 3 years ago

I'll try fork

thomashoneyman commented 3 years ago

All done! Thanks everyone!