-
Notifications
You must be signed in to change notification settings - Fork 66
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
TASK: Analyser for DB connector strings #79
Conversation
Pull Request Test Coverage Report for Build 95
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there. Thanks for your work. To have a consistent naming, could you please rename "Analyser" to "Analyzer" in the class name, name variable etc.? That way it would be uniform.
Thank you very much.
Sure will do it asap 👍
…On Thu, Oct 3, 2019, 2:43 PM Rico ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hi there. Thanks for your work. To have a consistent naming, could you
please rename "Analyser" to "Analyzer" in the class name, name variable
etc.? That way it would be uniform.
Thank you very much.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#79?email_source=notifications&email_token=AF34J42W2GBY3CHITYB6WADQMWZU5A5CNFSM4I4VRPP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGX5YBY#pullrequestreview-296737799>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF34J45C4CG5RMMIQBQIRODQMWZU5ANCNFSM4I4VRPPQ>
.
|
Also 1) don't worry about merge conflicts because of the and 2) is it intended that Ah and 3) don't stress yourself, you got all the time you need. This PR is not running away. Apart from that it matches connection strings very well! Have a great day 😀 |
Ok I will fix that http part. Thanks for the comments 👍
…On Thu, Oct 3, 2019, 2:49 PM Rico ***@***.***> wrote:
Also 1) *don't* worry about merge conflicts because of the
pastepwn/analyzers/__init__.py file. I am currently merging a lot of PRs
and will fix them myself :)
and 2) is it intended that http://regex101.com matches the analyzer?
Currently it would match every http URL. I would suggest turning
(?!https\b) to (?!http(s)?\b) which should fix it.
Ah and 3) don't stress yourself, you got all the time you need. This PR is
not running away.
Apart from that it matches connection strings very well! Have a great day
😀
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#79?email_source=notifications&email_token=AF34J45OPQRMJLAUE7MMBLLQMW2JLA5CNFSM4I4VRPP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAHSDQA#issuecomment-537862592>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF34J42D6XWFORBTRP3BQNLQMW2JLANCNFSM4I4VRPPQ>
.
|
This pull request is regarding the issue Database Connector analyzer #70