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 support for removing dependencies #202

Merged
merged 2 commits into from
Sep 23, 2023
Merged

add support for removing dependencies #202

merged 2 commits into from
Sep 23, 2023

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Sep 22, 2023

This PR updates desiutil.depend to add support for removing DEPNAMnn/DEPVERnn dependencies:

  • removedep(header, name): remove a single dependency by name
  • remove_dependencies(header): remove all dependencies

This also required some updates to getdep and iterdep to not trip on dependencies that had been removed, resulting in non-contiguous DEPNAMnn counting.

This will be used by desihub/desispec#2121 which needs to remove DEPNAM/DEPVER keywords from individual file inputs while stacking them so that they don't get incorrectly merged by astropy.table.vstack logic which doesn't know about DEPNAMnn/DEPVERnn pairs.

@coveralls
Copy link

coveralls commented Sep 22, 2023

Coverage Status

coverage: 74.401% (+0.09%) from 74.314% when pulling 0bd555b on remove_dependencies into 3037a6f on main.

@weaverba137
Copy link
Member

Are you still planning to add collect_dependencies() in this PR?

@sbailey
Copy link
Contributor Author

sbailey commented Sep 22, 2023

Are you still planning to add collect_dependencies() in this PR?

No. With the utility functions added here, the code in desihub/desispec#2101 would become

mergedep(data.meta, dependencies)
remove_dependencies(data.meta)

without needing a separate collect_dependencies function to wrap those (and two lines of "mergedep" + "remove_dependencies" seems clearer to me than wondering what "collect_dependencies" does and having to go read that code to find out...)

Copy link
Member

@weaverba137 weaverba137 left a comment

Choose a reason for hiding this comment

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

This all looks fine to me. Go ahead and merge when ready.

@sbailey sbailey merged commit f9bd0ae into main Sep 23, 2023
24 checks passed
@sbailey sbailey deleted the remove_dependencies branch September 23, 2023 04:21
sbailey added a commit that referenced this pull request Sep 23, 2023
@sbailey
Copy link
Contributor Author

sbailey commented Sep 23, 2023

Thanks. I merged and updated changes.rst in main.

weaverba137 added a commit that referenced this pull request Sep 26, 2023
* main:
  update dev version after 3.4.1 tag
  update release notes and version for desiutil/3.4.1
  update changelog for PR #202
  fix codestyle whitespace I think
  add support for removing dependencies
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.

3 participants