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

Add Black Requirement and Workflow Check #208

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Add Black Requirement and Workflow Check #208

merged 1 commit into from
Jun 10, 2022

Conversation

d33bs
Copy link
Member

@d33bs d33bs commented Jun 9, 2022

Description

This change adds a Black linting/formatting check as part of the Github workflow actions associated with this repo. This change is intended to address #200, making black formatting checks automatic and more visible for development collaboration.

What is the nature of your change?

  • Bug fix (fixes an issue).
  • Enhancement (adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • This change requires a documentation update.

Checklist

Please ensure that all boxes are checked before indicating that a pull request is ready for review.

  • I have read the CONTRIBUTING.md guidelines.
  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • New and existing unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have deleted all non-relevant text in this pull request template.

@d33bs d33bs requested a review from gwaybio June 9, 2022 17:26
@gwaybio
Copy link
Member

gwaybio commented Jun 9, 2022

It looks like elements of #203 are sneaking into this PR as well. Before I review, can you rebase black-linting-check to remove commits added in #203?

You likely just need to sync your fork with the current upstream develop

@d33bs d33bs added the enhancement New feature or request label Jun 9, 2022
@d33bs
Copy link
Member Author

d33bs commented Jun 9, 2022

Apologies and thank you @gwaybio . I've just rebased as you asked.

@@ -4,3 +4,4 @@ pandas>=1.2.0
scikit-learn>=0.21.2
sqlalchemy>=1.3.6
pytest>=5.0.1
black>=22.3.0
Copy link
Member

Choose a reason for hiding this comment

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

is it standard to include a package required for a workflow as a full package requirement? Is there another way to do this, or are you not concerned by adding an extra dependency in this case? (I sometimes get weary of adding dependencies!)

Copy link
Member Author

Choose a reason for hiding this comment

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

In my experience it's fairly common (though maybe not always perfect) to include development, testing, and linting tooling in a single requirements file for the repo. I think the idea is roughly that running and writing the code are close enough that they may be lumped together so as to not be lost. I might say the single requirements.txt file is an MVP - minimum viable Python for a Python project 🙂 .

That said, there are paradigms which expand away from single requirements files to allow for "non-production" dependency management, among other features (as "developer requirements" or other specialized tooling specific to environments or versioning). Maybe we can dig into some of these as part of another issue or part of #201 ? The increased complexity for any of these is usually I think a bit of a balancing act (time/code vs. environment isolation/reproducibility/etc). See below:

  • A simple way to do this is by using a requirements-dev.txt file (for ex., I happened to notice the Modin repo uses this pattern).
  • Dependency or environment managers:
    • These involve a bigger investment towards alternative file formats and additional dependencies developers must maintain at a benefit of improving things beyond what's provided with Python by default. In and of themselves they involve an additional learning curve and time for migration (to or from). The advantage is usually that developers have improved reproducibility when it comes to their code (where otherwise we rely on intuition/experience, IDE's, etc for this) and hopefully(!) less time spent attempting to resolve dependency conflicts where pip may fall short. Some examples below.
    • Poetry grouping allows specification of one or many groups of dependencies for whatever need may arise. My own personal experience lands more lately with Poetry, which I've found to be nice to work with and straightforward to install (but again, not without a learning curve).
    • There are a number of others in this category, such as pdm, pyenv, pipenv, pip-tools, etc. Some of these fall in and out of favor as time goes on, and also as communities change. OS differences seem also to play a hand in how well developers are able to take advantage of these. Towards this thought, containerization and compatibility within that idea is important to think about too.
  • There are some proposals within the Python community which maybe seek to address some of the ideas in your comment (see PEP 665 of July 2021). While it looks like this PEP was rejected, there's some interesting content to consider that may be relevant for guiding the way your projects develop. I wouldn't be surprised to find out that there's another PEP or ongoing work towards this - as noted it's something which many languages (or libraries) already do or have figured out along the way.

@gwaybio gwaybio self-requested a review June 10, 2022 15:18
Copy link
Member

@gwaybio gwaybio left a comment

Choose a reason for hiding this comment

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

Wonderful explanation - let's merge this in and not spend time on dependency management at the present moment. Thanks!

@d33bs d33bs merged commit ba99ff5 into cytomining:develop Jun 10, 2022
@d33bs d33bs deleted the black-linting-check branch June 10, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants