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 pyproject.toml #267

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

SylviaDu99
Copy link
Collaborator

WIP: add pyproject.toml, test_toml.py; edited setup.py to avoid overwriting with pyproject.toml

target to fix: issue #256

@MaxGhenis
Copy link
Contributor

My understanding was that we could replace setup.py entirely

@SylviaDu99
Copy link
Collaborator Author

My understanding was that we could replace setup.py entirely

I saw in the python documentations that setup.py could be kept, but I believe that getting rid of setup.py makes more sense. I would discuss with Anthony tomorrow about this and make changes if necessary.

@anth-volk
Copy link
Contributor

Met with @SylviaDu99 today and am in agreement that setup.py should go

@SylviaDu99 SylviaDu99 marked this pull request as ready for review September 7, 2024 04:17
@SylviaDu99
Copy link
Collaborator Author

Resolved setup.py conflict to enable CI/CD by fetching/rebasing

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 79.74%. Comparing base (cf53c6d) to head (588982e).

Files with missing lines Patch % Lines
tests/core/test_toml.py 83.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #267   +/-   ##
=======================================
  Coverage   79.73%   79.74%           
=======================================
  Files         192      193    +1     
  Lines       10057    10081   +24     
  Branches     1311     1315    +4     
=======================================
+ Hits         8019     8039   +20     
- Misses       1753     1756    +3     
- Partials      285      286    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

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

Thanks for all of your work on this @SylviaDu99.

Unfortunately, if you check the actions, even though they say they pass, the check_version_number action actually fails. Additionally, we've been facing challenges getting versioning to actually work with pyproject.toml (see this issue in the -us-data repository). This is obviously something we'll need to get working. That said, I don't think there's value to each working separately to get a versioning script working.

I think there are two ways we can proceed:

  • You dig deeper into getting this versioning script working: happy to hand it over to you
  • We allow this PR to sit and we choose something that would be more impactful and leave the switch to pyproject.toml alone for the time being

I'd lean more toward the second, as we're already facing pyproject.toml configuration-related issues on -us-data. Curious to hear your thoughts.

@MaxGhenis
Copy link
Contributor

I'd also favor putting things that aren't broken on hold for the moment until we stabilize the rest of the codebase. Really appreciate this work so far as it'll be a great way to modernize soon.

@SylviaDu99
Copy link
Collaborator Author

Thank you for your comments. I would also agree to move onto things that are more impactful.

@nikhilwoodruff nikhilwoodruff marked this pull request as draft September 18, 2024 20:13
@nikhilwoodruff
Copy link
Contributor

Thanks guys- converting to draft since not immediately mergeable.

@nikhilwoodruff nikhilwoodruff changed the title add pyproject.toml Add pyproject.toml Sep 18, 2024
@SylviaDu99 SylviaDu99 mentioned this pull request Oct 1, 2024
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