-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
fix: Fix URL Checker #3393
fix: Fix URL Checker #3393
Conversation
@Saransh-cpp I have created the PR. Requesting for your review. |
Thanks, @Agnik7! We can skip adding this in the CHANGELOG. |
Ok, I'm removing it from the CHANGELOG |
@agriyakhetarpal I have removed it. Please check. |
A request to the maintainers of this repository: I am participating in Hacktoberfest, so if you merge this PR, kindly add the hacktoberfest and hacktoberfest-accepted label to it. Apologies for any inconvenience caused. |
CONTRIBUTING.md
Outdated
@@ -185,7 +185,7 @@ You may also test multiple notebooks this way. Passing the path to a folder will | |||
nox -s examples -- docs/source/examples/notebooks/models/ | |||
``` | |||
|
|||
You may also use an appropriate [glob pattern](https://www.malikbrowne.com/blog/a-beginners-guide-glob-patterns) to run all notebooks matching a particular folder or name pattern. | |||
You may also use an appropriate [glob pattern](https://developers.tetrascience.com/docs/common-glob-pattern) to run all notebooks matching a particular folder or name pattern. |
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.
This seems like a link to another software package. Is there something more general that could be used? Like Linux documentation or python documentation?
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.
@kratman I have changed the link to a Linux documentation link. Is it alright? Please check
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.
Thanks @Agnik7, I can see that the linkChecker is still failing.
Can you please look into it? Maybe we can try with official python documentation
@arjxn-py Alright changing that right now. |
@arjxn-py I have changed the link. Please check. |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3393 +/- ##
========================================
Coverage 99.58% 99.58%
========================================
Files 256 256
Lines 19998 19998
========================================
Hits 19915 19915
Misses 83 83 ☔ View full report in Codecov by Sentry. |
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.
Thanks 🙂, looks good to me.
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.
Looks good to me, thanks!
@all-contributors please add @Agnik7 for documentation |
I've put up a pull request to add @Agnik7! 🎉 |
@Agnik7, going forward it is better to create a new branch in your fork rather than work on the develop branch, as this can cause issues. |
@brosaplanella alright! I'll surely keep that in mind |
Happy to merge once @kratman has checked the new changes :) |
Thanks a lot @brosaplanella for your guidance!!! Requesting @kratman to review this PR. Apologies for any inconvenience caused |
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.
Sorry, did not realize you guys were waiting on me.
This is one of the links I found when I was reviewing this initially and I think it is a reasonable one. The only other link I saw that seemed reputable was from a unix man pages project (man7.org), but python documentation is probably better for now.
Thanks a lot @brosaplanella and @kratman for your invaluable suggestions and guidance throughout!! Requesting @brosaplanella a follow up to this PR. Thanks a lot!!! |
@Agnik7 The change is approved, but the tests need to be re-triggered because your branch was updated. The tests take a few hours and will have to get approved before they start. |
@kratman Ok got it. Thanks a lot for the info!! |
@brosaplanella I have upstreamed this PR. Kindly approve the checks. Apologies for any inconvenience caused. |
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.
Sorry for taking too long to approve the tests. There are a couple of more failing URLs, could you please fix them in this PR itself?
- https://wigging.me should be changed to https://gavinw.me in
README.md
and here -
Line 445 in d02a78b
"profile": "https://wigging.me", |
- The hackerearth blog post should be replaced with any alternative blog post discussing Python for R users.
@Saransh-cpp alright, I'm on it!! |
@Saransh-cpp I have made the changes. Please have a look at them. |
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.
Looks good, thanks, @Agnik7!
Description
Fixed the broken glob pattern link in CONTRIBUTING.md. I have replaced the previous link with the following link
Fixes #3386
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all
(or$ nox -s tests
)$ python run-tests.py --doctest
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick
(or$ nox -s quick
).Further checks: