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 --additional-github-domain to check-vcs-permalinks #530

Merged
merged 1 commit into from
Nov 18, 2020
Merged

Add --additional-github-domain to check-vcs-permalinks #530

merged 1 commit into from
Nov 18, 2020

Conversation

youngminz
Copy link
Contributor

@youngminz youngminz commented Nov 16, 2020

I have added an option to inspect the source code of the additional domain, especially useful on GitHub Enterprise.


if args.github_enterprise_domain:
github_enterprise_non_permalink = re.compile(
NON_PERMALINK % args.github_enterprise_domain.encode(),
Copy link
Member

Choose a reason for hiding this comment

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

please don't use %s, if anything use .format() or f-strings -- thanks!

I notice this is a bytestring which doesn't support .format() or f-strings, but you can change it to not be a bytestring and then do .encode() afterwards as you've done for the domain

you can probably also inline the string as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the NON_PERMALINK is used only inside the function that converts the domain to a regular expression, I changed it to inline and used the f-string.

NON_PERMALINK % args.github_enterprise_domain.encode(),
)
for filename in args.filenames:
retv |= _check_filename(filename, github_enterprise_non_permalink)
Copy link
Member

Choose a reason for hiding this comment

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

this is going to re-parse twice, maybe add an additional argument to _check_filename such that it only runs over each file twice? this will also help with output ordering as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than increasing the logic branch, using a list can make it simpler, so I tried using a list. I tried modifying action='append' to allow users to add more than one domain. How about this?

b'https://example.com/asottile/test/blob/master/foo%20bar\n'
# regression test for overly-greedy regex
b'https://example.com/ yes / no ? /blob/master/foo#L1\n',
)
Copy link
Member

Choose a reason for hiding this comment

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

you can just use a simpler test, no need to re-test everything again

Copy link
Contributor Author

@youngminz youngminz Nov 18, 2020

Choose a reason for hiding this comment

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

Since the logic does not change even if the user specifies additional domains to check, I removed the previously wrote test cases and added a test case that fails. Are there any more test cases to add?

README.md Outdated
@@ -66,6 +66,7 @@ Attempts to load all TOML files to verify syntax.

#### `check-vcs-permalinks`
Ensures that links to vcs websites are permalinks.
- `--github-enterprise-domain DOMAIN` - Add check for specified GitHub Enterprise domain.
Copy link
Member

Choose a reason for hiding this comment

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

maybe --additional-domain is simpler? it doesn't necessarily have to be github enterprise, just something with similar url structure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied it!

@youngminz youngminz changed the title Add --github-enterprise-domain to check-vcs-permalinks Add --additional-domain to check-vcs-permalinks Nov 18, 2020
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

since it looks like your company is using pre-commit, maybe consider sponsoring (I'm currently unemployed!)

@asottile asottile merged commit 14e9f0e into pre-commit:master Nov 18, 2020
@youngminz
Copy link
Contributor Author

Thanks to pre-commit, our team can reduce unnecessary convention discussions and have more productive business logic discussions! I hope my sponsor is helpful to you.

@asottile
Copy link
Member

Thanks to pre-commit, our team can reduce unnecessary convention discussions and have more productive business logic discussions! I hope my sponsor is helpful to you.

<3 thanks! I really appreciate it!

@youngminz youngminz changed the title Add --additional-domain to check-vcs-permalinks Add --additional-github-domain to check-vcs-permalinks Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants