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

updated environment file #22

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Conversation

matt-long
Copy link

This PR includes additional packages that are likely to be useful in analysis.

While I appreciate the effort to delineate dependencies into categories, I find that in practice it's much easier to simply have an alphabetized list; this enable determining at a glance whether a particular package is included or not.

@andersy005, can you please review to make sure we're not missing anything essential or if we might need to pin any versions.

@matt-long matt-long requested a review from klindsay28 August 27, 2020 16:10
@matt-long
Copy link
Author

@mnlevy1981, black is complaining here about files I did not touch.

@mnlevy1981
Copy link
Contributor

@mnlevy1981, black is complaining here about files I did not touch.

I think black recently released v20, and it has slightly different expectations than v19... I ran into something similar in pop-tools; thanks for the heads up, I'll get it fixed on master

@andersy005
Copy link
Collaborator

I think black recently released v20, and it has slightly different expectations than v19... I ran into something similar in pop-tools; thanks for the heads up, I'll get it fixed on master

To keep everything in sync, we could replace the existing black GitHub action with the pre-commit hook check GitHub action. This guarantees that the settings and versions used by pre-commit hooks are the same as the ones used in the CI:

linting.yaml:

name: linting

on:
  push:
    branches: '*'
  pull_request:
    branches: '*'

jobs:
  linting:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-python@v2
      - uses: pre-commit/action@v2.0.0

It makes sense to add the above changes to #17.

Copy link
Contributor

@klindsay28 klindsay28 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I think it would be helpful to have done this is 2 commits, one with the rearranging the dependencies and another with adding dependencies. That would have made it easier to see that nothing was removed.

@matt-long
Copy link
Author

That's a good point.

@mnlevy1981, I'll let you merge when you're ready.

@mnlevy1981 mnlevy1981 mentioned this pull request Aug 28, 2020
@mnlevy1981 mnlevy1981 merged commit 8bc4fca into marbl-ecosys:master Aug 28, 2020
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.

4 participants