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

docs(examples/discounted-asset-trade): fix rendering of docs on website #3301

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

rajat-dlt
Copy link
Contributor

fix: Documentation fixed for cactus-example-discounted-asset-trade
At present, the documentation on the GitHub page isn't rendering correctly, and there are issues with numbering. Therefore, adjustments have been made to rectify these issues and ensure that the documentation functions as intended.

Signed-off-by: Rajat Sharma rajat16.sharma@ril.com

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

@rajat-dlt rajat-dlt changed the title Readme fixed for cactus-example-discounted-asset-trade fix: Documentation fixed for cactus-example-discounted-asset-trade Jun 7, 2024
@rajat-dlt
Copy link
Contributor Author

@petermetz Does this look fine or should I make any further changes in this ?

Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@rajat-dlt Thank you very much for this improvement! LGTM with comments (nit-picks):

  1. It's hard to untangle the actual changes from the white-space changes, could you please remove the white-space changes from the diff?
    If there are formatting issues it's OK to submit a separate pull request that includes only white-space and formatting changes but not logical changes. It's just not good to mix the two because then the review becomes much harder.
    For last, if the white-space changes are the fix then it's OK, but please let me know just to be sure.
  2. Please change the commit message and the PR title to something that doesn't qualify this as a production bug (the fix scope is reserved for problems that are affecting the software running in production). So my recommendation is to change it to docs(examples/discounted-asset-trade): fix rendering of docs on website

@outSH
Copy link
Contributor

outSH commented Jun 10, 2024

@rajat-dlt Also, please use prettier to format changed file according to root config ./.prettierrc.js

@rajat-dlt rajat-dlt changed the title fix: Documentation fixed for cactus-example-discounted-asset-trade docs(examples/discounted-asset-trade): fix rendering of docs on website Jun 10, 2024
@rajat-dlt rajat-dlt requested a review from petermetz June 10, 2024 12:50
@rajat-dlt
Copy link
Contributor Author

@petermetz ,

  1. The changes are the whitespaces and at the last one extra ``` that had to be removed.

  2. Updated the commit message, thanks for helping me out with this.

  3. @outSH resolved all the prettier issues in the file. Thanks for suggesting that.

Let me know if any further changes are required.

Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@rajat-dlt We are getting closer but there are still some things:

  1. You've pushed a second commit, please squash them together so that the pull request contains only a single commit.
  2. You haven't amended the git commit message, just edited the GitHub PR title but they are separate things:
    image

If you need any help with the git mechanics you can also join our daily pair programming call on the Hyperledger Discord server that runs at 10 AM, Pacific Time on weekdays: https://wiki.hyperledger.org/display/cactus/Daily+Pair+Programming+Calls
There you can share your screen if necessary and I can help you with the details!

@rajat-dlt
Copy link
Contributor Author

rajat-dlt commented Jun 10, 2024

@petermetz,
I've tried to squash and merge both the commits into 1. Hopefully this should be fine, otherwise I'll join the call to get this issue rectified. Thanks a lot for helping me so much with this.

I was expecting from the portal all the commits that are made and squashed and merged into 1 commit. Thus I improved the description here and then went on to make improvements by making multiple commits in the same PR. Apologies for the oversight.

@petermetz
Copy link
Member

@petermetz, I've tried to squash and merge both the commits into 1. Hopefully this should be fine, otherwise I'll join the call to get this issue rectified. Thanks a lot for helping me so much with this.

I was expecting from the portal all the commits that are made and squashed and merged into 1 commit. Thus I improved the description here and then went on to make improvements by making multiple commits in the same PR. Apologies for the oversight.

@rajat-dlt No worries, glad I could help! 2 things:

  1. You pushed a merge commit so now there are 2 commits in the log once again. Please squash them together when you get the chance!
  2. Next time after you updated the pull request please re-request the review otherwise this doesn't show up on my to-do list (I query the pull requests based on which one requires my review)

@rajat-dlt
Copy link
Contributor Author

@petermetz ,
Fixed the changes, was facing issues while squashing the commits initially so fixed the issues and pushed all the changes together in one commit.

@rajat-dlt
Copy link
Contributor Author

@outSH @petermetz ,
This PR is still not merged. Is there anything I should do further on this.

@outSH
Copy link
Contributor

outSH commented Jun 17, 2024

@rajat-dlt All good from my side, we're waiting for Peter approval :)

@petermetz
Copy link
Member

petermetz commented Jun 17, 2024

@outSH @petermetz , This PR is still not merged. Is there anything I should do further on this.

@rajat-dlt Correct. We try to review everything as soon as possible, but it can easily take a few days. With that said, it's looking good now, thank you for the contribution!

Signed-off-by: Rajat Sharma <rajat16.sharma@ril.com>
@petermetz petermetz merged commit b8a0fd5 into hyperledger:main Jun 18, 2024
147 of 150 checks passed
@rajat-dlt rajat-dlt deleted the readme-updated branch June 27, 2024 06:34
@rajat-dlt
Copy link
Contributor Author

Closes issue #3335 .

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.

None yet

3 participants