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

fix: Fix URL Checker #3393

Merged
merged 12 commits into from
Oct 8, 2023
Merged

fix: Fix URL Checker #3393

merged 12 commits into from
Oct 8, 2023

Conversation

Agnik7
Copy link
Contributor

@Agnik7 Agnik7 commented Oct 2, 2023

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.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ 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)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ 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:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 2, 2023

@Saransh-cpp I have created the PR. Requesting for your review.

@Agnik7 Agnik7 mentioned this pull request Oct 2, 2023
@agriyakhetarpal
Copy link
Member

Thanks, @Agnik7! We can skip adding this in the CHANGELOG.

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 2, 2023

Ok, I'm removing it from the CHANGELOG

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 2, 2023

@agriyakhetarpal I have removed it. Please check.

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 2, 2023

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.
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Member

@arjxn-py arjxn-py left a 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

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 2, 2023

@arjxn-py Alright changing that right now.

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 2, 2023

@arjxn-py I have changed the link. Please check.

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d02a78b) 99.58% compared to head (bc909c3) 99.58%.
Report is 30 commits behind head on develop.

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@arjxn-py arjxn-py left a 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.

@Agnik7 Agnik7 requested a review from kratman October 2, 2023 17:09
Copy link
Member

@brosaplanella brosaplanella left a 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!

@brosaplanella
Copy link
Member

@all-contributors please add @Agnik7 for documentation

@allcontributors
Copy link
Contributor

@brosaplanella

I've put up a pull request to add @Agnik7! 🎉

@brosaplanella
Copy link
Member

@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.

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 3, 2023

@brosaplanella alright! I'll surely keep that in mind

@brosaplanella
Copy link
Member

Happy to merge once @kratman has checked the new changes :)

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 3, 2023

Thanks a lot @brosaplanella for your guidance!!! Requesting @kratman to review this PR. Apologies for any inconvenience caused

Copy link
Contributor

@kratman kratman left a 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.

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 4, 2023

Thanks a lot @brosaplanella and @kratman for your invaluable suggestions and guidance throughout!! Requesting @brosaplanella a follow up to this PR. Thanks a lot!!!

@kratman
Copy link
Contributor

kratman commented Oct 4, 2023

@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.

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 4, 2023

@kratman Ok got it. Thanks a lot for the info!!

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 7, 2023

@brosaplanella I have upstreamed this PR. Kindly approve the checks. Apologies for any inconvenience caused.

Copy link
Member

@Saransh-cpp Saransh-cpp left a 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?

  1. https://wigging.me should be changed to https://gavinw.me in README.md and here -

"profile": "https://wigging.me",

  1. The hackerearth blog post should be replaced with any alternative blog post discussing Python for R users.

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 8, 2023

@Saransh-cpp alright, I'm on it!!

@Agnik7
Copy link
Contributor Author

Agnik7 commented Oct 8, 2023

@Saransh-cpp I have made the changes. Please have a look at them.

Copy link
Member

@Saransh-cpp Saransh-cpp left a 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!

@Saransh-cpp Saransh-cpp merged commit 535c034 into pybamm-team:develop Oct 8, 2023
33 checks passed
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.

Fix the URL checker
6 participants