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 documentation of query namespaces. #115

Merged
merged 2 commits into from
Feb 26, 2021
Merged

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 10, 2021

Description

Add documentation for query filters that combine state point and document keys.

Motivation and Context

Accompanies glotzerlab/signac#332

Checklist:

@vyasr vyasr requested review from a team as code owners February 10, 2021 18:21
@vyasr vyasr requested review from csadorf and klywang and removed request for a team February 10, 2021 18:21
Copy link
Contributor

@klywang klywang 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 clear to me!

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Is using doc_filter arguments then officially deprecated?

@vyasr
Copy link
Contributor Author

vyasr commented Feb 15, 2021

@csadorf I think doc_filter has been around long enough that I don't want to deprecate it in 2.0. I would consider maybe just leaving it undocumented going forward so that new users all switch to the new querying syntax. We could revisit deprecating in 3.0/4.0. Does that sound reasonable?

docs/source/query.rst Outdated Show resolved Hide resolved
docs/source/query.rst Outdated Show resolved Hide resolved
@bdice
Copy link
Member

bdice commented Feb 16, 2021

@vyasr @csadorf I'm not opposed to removing doc_filter in signac 2.0. I don't feel strongly that we should remove it but it would be okay with me if it provided us with maintenance benefits / API unification.

@csadorf
Copy link
Contributor

csadorf commented Feb 16, 2021

@vyasr @csadorf I'm not opposed to removing doc_filter in signac 2.0. I don't feel strongly that we should remove it but it would be okay with me if it provided us with maintenance benefits / API unification.

@bdice This is such core API that I think it would present a strong burden for migration. So, if we did this then I would suggest we explore providing some tooling (for example using Bowler) to automatically migrate existing projects. Basically rewrite (doc_filter={'key': 'value'}) to filter={'doc.key': 'value'} in the spirit of 2to3.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 16, 2021

I'm inclined to punt on removing/providing migration tools until a 3.0 release since I do think that it will be a significant migration burden for old scripts. My preference would simply be to preferentially document the new API going forward so that new scripts do not use this, making it more likely that by the time of a 3.0 release the migration would be a much lighter burden since scripts using doc_filter would likely be outdated and unused anyway at that point. We could conceivably deprecate in 2.0 and mark for removal in 3.0, this is the rare case where I would be OK with a deprecation without removal in a major release.

Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@vyasr vyasr merged commit aea42f7 into master Feb 26, 2021
@vyasr vyasr deleted the feature/integrated_queries branch February 26, 2021 16:41
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