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 processors class docs hyperlink (issue #593) #903

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

mjbear
Copy link
Contributor

@mjbear mjbear commented Apr 1, 2024

fix processors class docs hyperlink (issue #593)

@@ -6,7 +6,7 @@
"source": [
"# Processors\n",
"\n",
"Processors are plugins that can execute code on certain events. For more information on those events check the [class documentation](../../ref/api/processors.rst).\n",
"Processors are plugins that can execute code on certain events. For more information on those events check the [class documentation](https://nornir.readthedocs.io/en/latest/api/nornir/core/processor.html#nornir.core.processor.Processor).\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think this should be ../api/processors.rst

Copy link
Contributor Author

@mjbear mjbear Apr 6, 2024

Choose a reason for hiding this comment

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

Thank you for the response.

Oops on my part. Sphinx auto-generated docs rather than hard-coded hyperlinks makes sense. 😅

@mjbear
Copy link
Contributor Author

mjbear commented Apr 6, 2024

Updated the PR with a relative link to processor.rst

Edit: processors, plural 😅

@mjbear
Copy link
Contributor Author

mjbear commented Apr 21, 2024

@dbarrosop I believe this is to be fixed.

@dbarrosop
Copy link
Contributor

dbarrosop commented Apr 23, 2024

Argh, this may need a similar fix to the other one you submitted in the other repo, not sure if you'd like to fix this one as well (it'd be very appreciated :P)

@mjbear
Copy link
Contributor Author

mjbear commented Aug 4, 2024

Argh, this may need a similar fix to the other one you submitted in the other repo, not sure if you'd like to fix this one as well (it'd be very appreciated :P)

Hello @dbarrosop
Apologies for forgetting about this when I got busy.
Looks like I lost the other thread/repo reference.

I suspect the page to link to is: https://nornir.readthedocs.io/en/latest/api/nornir/core/processor.html which corresponds to https://github.com/nornir-automation/nornir/blob/main/docs/api/nornir/core/processor.rst
Right?
So the relative path would be ../api/nornir/core/processor.rst

❓ I'm trying to regenerate the docs with sphinx so I can look at them before I push any updates.
I installed sphinx, but when I try to run make docs it throws an exception that it cannot find the module.

Once I correct this broken link then I will chase after the list of links I found so I can correct them too. 😉

@coveralls
Copy link

coveralls commented Aug 5, 2024

Pull Request Test Coverage Report for Build 10276617225

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 56.813%

Totals Coverage Status
Change from base Build 10263735640: 0.0%
Covered Lines: 542
Relevant Lines: 954

💛 - Coveralls

@dbarrosop
Copy link
Contributor

dbarrosop commented Aug 5, 2024

No worries, this was fixed already so we only had to rebase.

I installed sphinx, but when I try to run make docs it throws an exception that it cannot find the module.

try doing git pull to pull the commit I pushed to your branch and trying again, the issue should be fixed now

Btw, do you want me to merge the PR or do you want to commit more changes here?

@mjbear
Copy link
Contributor Author

mjbear commented Aug 5, 2024

No worries, this was fixed already so we only had to rebase.

Interesting. I fetched from your repo's main branch (July 23rd commits) and rebased my "feature" branch in the past few days.

commit d35cb58c9c462d5c1dd8658944de27f1c0824503 (HEAD -> docs_processors_link)
Author: Michael Bear <38406045+mjbear@users.noreply.github.com>
Date:   Sat Apr 6 00:31:58 2024 -0400

    processors, plural

commit 2a4aa91a5d593d726195dc95dc7a385fd8b4dc6f
Author: Michael Bear <38406045+mjbear@users.noreply.github.com>
Date:   Sat Apr 6 00:11:33 2024 -0400

    using relative link instead of hardcoded url

commit 76dce28268da579c071233acd96c9add9b2524ae
Author: Michael Bear <38406045+mjbear@users.noreply.github.com>
Date:   Sun Mar 31 21:24:24 2024 -0400

    fix processors class docs hyperlink (issue #593)

commit d8e82d4c959d3ae43fc4a9c9a51737e71e77e871 (upstream/main, main)
Author: Patrick Ogenstad <patrick@REMOVED>
Date:   Tue Jul 23 20:11:37 2024 +0200

    Simlify _verify_rules and redunce max-complexity=10 (#962)
    
    Co-authored-by: David Barroso <dbarrosop@REMOVED>

I installed sphinx, but when I try to run make docs it throws an exception that it cannot find the module.

try doing git pull to pull the commit I pushed to your branch and trying again, the issue should be fixed now

From the commit history that steam rolled any commits I had made.
I used the reflog to roll my local copy back so I can look around.

Is the intent that I should lay my changes on top of the merge you made on my feature branch?
(Apologies 😓, I thought my rebase from earlier would have got my branch current to upstream/main.)

Btw, do you want me to merge the PR or do you want to commit more changes here?

When it's ready, my thought is to merge this one and I'll build up other PR(s).

@dbarrosop
Copy link
Contributor

Is the intent that I should lay my changes on top of the merge you made on my feature branch?

It doesn't really matter, as long as there are no conflicts it doesn't matter if you rebase or push changes on top or whatever, we will end up squashing the commits into and keeping the history linear.

When it's ready, my thought is to merge this one and I'll build up other PR(s).

Ok, let me know when you think it's ready

@mjbear mjbear closed this Aug 7, 2024
@mjbear mjbear deleted the docs_processors_link branch August 7, 2024 00:51
@mjbear mjbear restored the docs_processors_link branch August 7, 2024 00:51
@mjbear mjbear reopened this Aug 7, 2024
@mjbear
Copy link
Contributor Author

mjbear commented Aug 7, 2024

@dbarrosop
I think this is ready.

Based on what I saw in the inventory tutorial I realized there were still some changes I should make. Along with that I squashed my commit history.

I need to learn more about sphinx since I want to locally test before pushing code to GH. 🤔 😀

@dbarrosop dbarrosop merged commit cc31296 into nornir-automation:main Aug 7, 2024
16 checks passed
@dbarrosop
Copy link
Contributor

Thanks!

@mjbear mjbear deleted the docs_processors_link branch August 7, 2024 14:06
@mjbear
Copy link
Contributor Author

mjbear commented Aug 7, 2024

Thanks!

And thank you for your patience! 😅
I learn things by contributing and some of my learning is by making mistakes. 🧗‍♂️ ⛰️

@mjbear mjbear mentioned this pull request Aug 14, 2024
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.

3 participants