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

Remove bumpversion and use bump-my-version #90

Merged
merged 10 commits into from
Mar 5, 2024
Merged

Conversation

R-Palazzo
Copy link
Contributor

@R-Palazzo R-Palazzo commented Mar 4, 2024

CU-86azg1dg3
Resolve #87

@amontanez24 @gsheni SDV has a version folder while our other libraries define their __version__ inside their __init__.py directly, is it fine?

@R-Palazzo R-Palazzo requested a review from a team as a code owner March 4, 2024 13:42
@sdv-team
Copy link
Contributor

sdv-team commented Mar 4, 2024

@R-Palazzo R-Palazzo removed the request for review from a team March 4, 2024 13:42
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.12%. Comparing base (8da3fed) to head (906a134).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   78.98%   79.12%   +0.13%     
==========================================
  Files           7        7              
  Lines         728      728              
==========================================
+ Hits          575      576       +1     
+ Misses        153      152       -1     

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

pyproject.toml Outdated
Comment on lines 104 to 116
[tool.setuptools.package-data]
"*" = [
'*.txt',
'*.md',
'*.rst',
'README.md',
'docs/*',
'Makefile',
'make.bat',
'*.jpg',
'*.png',
'*.gif',
]
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we adding this now?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • This specifies, what should be included in the whl/tar.gz file (when making the package). We should keep this as minimal as possible (to avoid making our released package too large).
  • One thought is we should not include the docs folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the *.txt? It seems like our MANIFEST doesn't include it anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

With pyproject.toml, you can actually remove MANIFEST.in file.

All files specified by the package-data and data-files configuration parameters in pyproject.toml and/or equivalent in setup.cfg/setup.py;
https://setuptools.pypa.io/en/latest/userguide/miscellaneous.html

Copy link
Contributor

Choose a reason for hiding this comment

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

@R-Palazzo will approve once MANIFEST.in is removed.

@gsheni
Copy link
Contributor

gsheni commented Mar 5, 2024

@R-Palazzo regarding the version folder. Yes that is fine.
Ideally, we follow one versioning for all of our libraries (my preference is 1 version.py file in the root of the project (no subfolder)

pyproject.toml Outdated
@@ -15,7 +15,7 @@ classifiers = [
'Topic :: Scientific/Engineering :: Artificial Intelligence',
]
keywords = ['deepecho', 'DeepEcho']
dynamic = ["version"]
version = '0.5.1.dev0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's follow what we did with Copulas:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I was not sure, I checked with @amontanez24. Done in b1fa699

Copy link
Contributor

@gsheni gsheni left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

:shipit:

@R-Palazzo R-Palazzo merged commit f2a6394 into main Mar 5, 2024
45 checks passed
@R-Palazzo R-Palazzo deleted the issue-87-bump-my-version branch March 5, 2024 19:07
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.

Remove bumpversion and use bump-my-version
5 participants