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 query-string construction helpers #84

Closed
wants to merge 6 commits into from

Conversation

lukasschwab
Copy link
Owner

@lukasschwab lukasschwab commented Aug 31, 2021

Description

Operator methods are capitalized because def and() and def or() are invalid Python: the tokens clash with Python's boolean operators. Considered _and, _or, but these have additional semantic meaning.

Breaking changes

List any changes that break the API usage supported on master.

Expands, but doesn't break, the API.

  • Adds an automatically-imported query module.
  • Adds a category module.

Relevant issues

List GitHub issues relevant to this change.

Fix #30

Checklist

  • Add docstrings
  • Tidy script for regenerating categories enum
  • (If appropriate) README.md example usage has been updated. Should think about whether to add examples; the README is a somewhat unwieldy way to manage documentation.

@lukasschwab lukasschwab force-pushed the fix-30-add-query-string-helpers branch from 4e09d3d to 6c07450 Compare August 31, 2021 04:46
@lukasschwab lukasschwab linked an issue Aug 31, 2021 that may be closed by this pull request
@lukasschwab lukasschwab changed the title Fix #30: add query string construction helpers Add query-string construction helpers Aug 31, 2021
@lukasschwab
Copy link
Owner Author

@mhils don't feel obligated to review my code, but my work here reminded me of your comments about project structure on #68.

This PR adds another module (arxiv.query), which is reexported in __init__.py alongside arxiv.arxiv. If you're interested in weighing in, I'd appreciate your feedback; does this feel like an antipattern?

@mhils
Copy link
Contributor

mhils commented Sep 9, 2021

It looks like you want to publicly expose your entire API on the top-level arxiv namespace, but you want to split up the code internally into different submodules ? In that case I personally would do what httpx does: Explicit imports + __all__ + internal modules prefixed with an underscore.

https://github.com/encode/httpx/blob/master/httpx/__init__.py

There really is no super well-established convention here though, it boils down to personal preference a lot. I guess the only thing I would avoid is * imports, it makes it hard for trace where things are actually coming from. For example when I'm interested in the definition of QuantitativeBiology, __init__.py currently has no hint on which submodule I need to look into. Does that make sense?

@lukasschwab
Copy link
Owner Author

@mhils yes, that makes sense––thanks for the input!

It looks like you want to publicly expose your entire API on the top-level arxiv namespace, but you want to split up the code internally into different submodules?

Yeah, that's about right! With one complication: if possible, I want to avoid breaking the original import arxiv usage.

If I were writing this package from scratch, I'd use named submodules:

  • arxiv.api (the original module, currently named arxiv.arxiv)
  • arxiv.query (unchanged)
  • arxiv.fixtures module for consts/enums defined by arXiv themselves. This is porbably a better name than arxiv.categories... I'll think about renaming in this PR.

The problem is that the current organization encourages import arxiv, but also permits from arxiv import arxiv.

I'm inclined to only allow explicit imports, but there are two issues:

  • import arxiv usage is broken.
  • I'd like to rename arxiv.arxiv to arxiv.api to avoid from arxiv import arxiv, but that's a breaking change.

I suppose I could move the API logic into an arxiv.api module; then––to avoid breaking changes––a deprected arxiv.arxiv module can * export arxiv.api, and __init__.py can auto-export arxiv.arxiv.

@lukasschwab
Copy link
Owner Author

Oh, looks like I jumped to conclusions! I can turn arxiv.query and arxiv.arxiv internal (arxiv._query and arxiv._arxiv respectively) and have init.py explicitly import certain values from them, like httpx does.

I'll think more about this.

@lukasschwab lukasschwab force-pushed the fix-30-add-query-string-helpers branch from 94ef239 to ab29349 Compare January 26, 2022 03:12
@lukasschwab
Copy link
Owner Author

Closing: stale.

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.

Query string helpers
2 participants