Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
20 tasks done
thomashoneyman opened this issue Jul 27, 2021 · 10 comments
Closed
20 tasks done

Comments

@thomashoneyman
Copy link
Contributor

thomashoneyman commented Jul 27, 2021

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
Copy link

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
Copy link

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
Copy link

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

@ntwilson
Copy link

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

@thomashoneyman
Copy link
Contributor Author

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
Copy link

pete-murphy commented Jul 28, 2021

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

@ntwilson
Copy link

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
Copy link
Contributor Author

thomashoneyman commented Jul 28, 2021

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
Copy link

I'll try fork

@thomashoneyman
Copy link
Contributor Author

All done! Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants