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 user-provided project name from API #677

Merged
merged 13 commits into from
Feb 21, 2022
Merged

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 4, 2022

Description

This PR removes the ability for users to set a project name. The name still exists, but it is always just "project". These changes were split off from #643. Currently this changeset only includes the minimal set of changes to ensure that tests pass, but a large number of tests will now trigger warnings. Once reviewers indicate their approval of the new API I can go through and clean up tests.

Specifically, the changes here ensure that in 2.0 a user who accidentally passes a project name as either a positional or keyword argument will only see a warning, not an error. This choice is intended to provide a softer off-ramp for project names since they have been an integral part of signac since its inception. The only cases that were previously valid that will now fail are if the user provides multiple positional arguments in order to set the root directory, e.g. init_project("project_name", "/root/directory"). I think that this is an acceptable level of breakage since the vast majority of users do not set the root explicitly.

I haven't removed the Project.id property here since the name still exists and users can still access it until the 2.0 migration actually removes it. I will remove the id property at that time.

Motivation and Context

Prep for 2.0. Part of #541.

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

If necessary:

  • I have updated the API documentation as part of the package doc-strings.
  • I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
  • I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

@vyasr vyasr added the requires-1.x-deprecation Changes for 2.0 that require a deprecation to be added in 1.x. label Feb 4, 2022
@vyasr vyasr added this to the v2.0.0 milestone Feb 4, 2022
@vyasr vyasr requested a review from a team as a code owner February 4, 2022 17:49
@vyasr vyasr self-assigned this Feb 4, 2022
@vyasr vyasr requested a review from a team as a code owner February 4, 2022 17:49
@vyasr vyasr requested review from bdice and SchoeniPhlippsn and removed request for a team February 4, 2022 17:49
@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #677 (6d79ac9) into next (2544101) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #677      +/-   ##
==========================================
+ Coverage   85.82%   85.84%   +0.01%     
==========================================
  Files          51       51              
  Lines        5024     5038      +14     
  Branches     1104     1109       +5     
==========================================
+ Hits         4312     4325      +13     
- Misses        515      516       +1     
  Partials      197      197              
Impacted Files Coverage Δ
signac/__main__.py 75.75% <100.00%> (-0.29%) ⬇️
signac/contrib/project.py 89.44% <100.00%> (+0.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2544101...6d79ac9. Read the comment docs.

signac/common/config.py Outdated Show resolved Hide resolved
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Suggestions attached.

signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
@vyasr vyasr force-pushed the refactor/project_naming branch 2 times, most recently from cf9c79a to bf4771f Compare February 10, 2022 07:26
signac/contrib/project.py Outdated Show resolved Hide resolved
tests/test_project.py Show resolved Hide resolved
signac/contrib/project.py Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
@vyasr vyasr requested review from csadorf and bdice February 12, 2022 20:55
Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

This looks good to me!

edit: I see Python 3.6 tests are failing. The underlying problem is that version.parse only has the attribute major for packaging >= 20.0. We pin Python 3.6 to packaging==15.0. This can be fixed by using version.parse(__version__).release[0] instead of version.parse(__version__).major.

https://packaging.pypa.io/en/latest/changelog.html#id14 (the linked issue is wrong, it should point to pypa/packaging#225)

@vyasr
Copy link
Contributor Author

vyasr commented Feb 13, 2022

@bdice thanks for debugging that for me and making the change painless!

@bdice
Copy link
Member

bdice commented Feb 13, 2022

@vyasr I think the method I proposed above would be a bit less hacky with version parsing but I approve the fix either way.

signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Show resolved Hide resolved
tests/test_project.py Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from csadorf February 18, 2022 15:59
signac/contrib/project.py Outdated Show resolved Hide resolved
signac/contrib/project.py Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from csadorf February 19, 2022 18:11
@vyasr
Copy link
Contributor Author

vyasr commented Feb 19, 2022

@csadorf feel free to merge this if you think it's ready.

@csadorf csadorf merged commit 30aa6a4 into next Feb 21, 2022
@csadorf csadorf deleted the refactor/project_naming branch February 21, 2022 09:35
@csadorf
Copy link
Contributor

csadorf commented Feb 21, 2022

Thanks @vyasr !

vyasr added a commit that referenced this pull request Feb 21, 2022
@vyasr vyasr mentioned this pull request Feb 21, 2022
12 tasks
vyasr added a commit that referenced this pull request Feb 21, 2022
@vyasr vyasr mentioned this pull request Mar 4, 2022
vyasr added a commit that referenced this pull request Mar 14, 2022
vyasr added a commit that referenced this pull request Apr 19, 2022
vyasr added a commit that referenced this pull request Apr 21, 2022
vyasr added a commit that referenced this pull request May 2, 2022
bdice pushed a commit that referenced this pull request Aug 1, 2022
bdice pushed a commit that referenced this pull request Oct 7, 2022
vyasr added a commit that referenced this pull request Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires-1.x-deprecation Changes for 2.0 that require a deprecation to be added in 1.x.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants