10000 Fix link bugs in readme.(#1025) by SuccinctPaul · Pull Request #1025 · pyrsia/pyrsia · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix link bugs in readme.(#1025) #1025

New issue

Have a question 8000 about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 12, 2022
Merged

Fix link bugs in readme.(#1025) #1025

merged 6 commits into from
Sep 12, 2022

Conversation

SuccinctPaul
Copy link
Contributor
@SuccinctPaul SuccinctPaul commented Aug 30, 2022 •
edited
Loading

Description

Ref #1018

Screenshots (optional)

PR Checklist

Code Contributions

  • I've built the code cargo build --all-targets successfully.
  • I've run the unit tests cargo test --workspace and everything passes.
  • I've made sure my rust toolchain is current rustup update.

@SuccinctPaul SuccinctPaul requested a review from a team as a code owner August 30, 2022 01:51
@SuccinctPaul SuccinctPaul requested review from betarelease and AbhijithGanesh and removed request for a team August 30, 2022 01:51
@SuccinctPaul SuccinctPaul mentioned this pull request Aug 30, 2022
Closed
7 tasks
@betarelease
Copy link
Member

Thanks for the quick fix @ChengYueJia.
I think you found an issue in the way we organize our docs. We are actually taking the docs/ folder and parsing it to our website https://pyrsia.io.

The links you fixed were indeed broken when you navigate through github project. And your fixes fix them for that view, but it breaks on the website (when I tried manually). I think your PR/suggestions need to be addressed but let me try a couple of options and we can merge this by editing a little bit.

@SuccinctPaul
Copy link
Contributor Author

@betarelease I'll waiting for the good news~~

AbhijithGanesh
AbhijithGanesh reviewed Aug 31, 2022
View reviewed changes
Copy link
Member
@AbhijithGanesh AbhijithGanesh left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @ChengYueJia but I don't think these changes affect/change the present readme file! So we might have to close the PR

@AbhijithGanesh
Copy link
Member

@betarelease This PR doesn't introduce any significant changes, closing the PR!

@betarelease
Copy link
Member

I will keep this open for a little. Basically we need to take in the changes that @ChengYueJia is suggesting. Just that they work on github and dont work that well on the website or vice versa. I need to test this on website and then confirm that we can take this in.

@betarelease betarelease reopened this Aug 31, 2022
@betarelease
Copy link
Member

@ChengYueJia I have not had a chance to test this yet. But will get to it today. (I need to run jekyll and then import a branch of pyrsia so needs a little setup to test).

In the meanwhile, please join our slack channel and meet the team. Since you are interested in Rust we have a number of areas you can start contributing to.

@SuccinctPaul
Copy link
Contributor Author

@betarelease Thanks for invitation. I'm glad to join in :).

@betarelease
Copy link
Member

Hi @ChengYueJia

I tested this with the generation of webpages from our docusaurus website.

Basically I had to test it in two ways as follows:

  1. Ensure that these links are indeed broken as you observed on github itself and some of them are. Thanks for catching.
  2. Then to verify the proposed fix I built the website by from github.com/pyrsia/pyrsia.github.io repo and used my local pyrsia/ repo to push latest changes to it. Then bring up the website using the instructions on github.com/pyrsia/pyrsia.github.io. Verify that each of those changes work as expected.

I left comments inline to each of your changes and I think we can take 3 of the 4 changes as you made them. Can you please update the PR and I can approve it for merge.

Thanks for your patience. It was a little tricky to test this out since we have not written down the steps, but I will write them down now.

betarelease
betarelease reviewed Sep 8, 2022
View reviewed changes
@SuccinctPaul
Copy link
Contributor Author

@betarelease Thanks for your patience and guide.
I've done the modifies.

@betarelease betarelease merged commit a0fe1c2 into pyrsia:main Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbhijithGanesh AbhijithGanesh AbhijithGanesh left review comments

@betarelease betarelease betarelease approved these changes

@fishseabowl fishseabowl fishseabowl approved these changes

@dasmanas dasmanas Awaiting requested review from dasmanas

Assignees

@SuccinctPaul SuccinctPaul

Labels
in-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SuccinctPaul @betarelease @AbhijithGanesh @fishseabowl
0