GitHub: use_commit_name option
#213
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a ` : ` (space, colon, space) like that can be confused for a definition list classifier, especially in a definition list, so prefer to backslash-escape the colon. see the "reStructedText Markup Specification", section "Syntax Details" > "Body Elements" > "Definition Lists".
the `asyncio_mode` of `legacy` (current default) is deprecated. `asyncio_mode` will be `strict` by default in the future.
there is currently not GraphQL API equivalent to the REST API's
`/repos/{owner}/{repo}/commits/{ref}` endpoint.
lilydjwg
reviewed
Feb 23, 2022
| elif isinstance(x, dict): | ||
| return tuple(sorted((_normalize(k), _normalize(v)) for k, v in x.items())) | ||
| else: | ||
| return x |
There was a problem hiding this comment.
It's not obvious what this function does. It needs a docstring or a better name.
| _, url, headers = key | ||
| res = await session.get(url, headers=dict(headers)) | ||
| _, url, headers, json = key | ||
| json = extra # denormalizing json would be a pain, so we sneak it through |
There was a problem hiding this comment.
What about passing json as a str or bytes? We can set the Content-Type header and directly pass it to session as body
| @@ -393,6 +393,12 @@ use_latest_tag | |||
|
|
|||
| This requires a token because it's using the v4 GraphQL API. | |||
|
|
|||
| use_commit_name | |||
There was a problem hiding this comment.
What about use_commit_hash? It's not obvious what a commit name is.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I would like to use
nvcheckerto pin check for new Git commits ongithubwithout using thegitsource; this solves that problem. TheVER+HASHsyntax is compatible with Semantic Versioning, for whatever that's worth.Also, wider use of the GraphQL API when possible; it's a bit faster than the REST API and I had to use it more for comprehensive
use_commit_namesupport.nvchecker.util.AsyncCache.get_json()now knows how to send POST requests when ajsonbody parameter is provided; this makes using the GraphQL API more innvchecker_sources.githubeasier and more aligned with REST API.I'm not in love with the
list_countoption, but I didn't want to break backwards compatibility and I didn't want to be stuck fetching up to approximately 99 commits I don't need if I useuse_max_tagin the future. A good way to handle this might be issuing a deprecation notice whenlist_countisn't specified along withuse_max_tagin the future, and then removing thelist_count = 100default so it can be1like everywhere else. Talking about other uses oflist_count, there is flexibility to makeuse_latest_tagsupport list options; I did not expose this functionality in this patch set and it still returns a scalarstrfor now.See the individual commit diffs for a smoother progression that doesn't change basically all of
nvchecker_sources.githubat once.Based off #212 because I made sure each commit in this set passed tests.