Skip to content

Enforce code style on doc comments #156#158

Merged
wucke13 merged 2 commits intorosenpass:mainfrom
AlooDon:Enforce-code-style-on-doc-comments-#156
Dec 22, 2023
Merged

Enforce code style on doc comments #156#158
wucke13 merged 2 commits intorosenpass:mainfrom
AlooDon:Enforce-code-style-on-doc-comments-#156

Conversation

@alankritdabral
Copy link
Copy Markdown

@alankritdabral alankritdabral commented Nov 20, 2023

Summary:
Added format_rust_code.sh file that uses rustfmt tool on rust code in fences.
to check document formatting run: run bash format_rust_code.sh --mode check
to fix run : run bash format_rust_code.sh --mode fix

modifie qc.yml: To run format_rust_code.sh

Issue References:
Fixes: #156
/claim #156

Screenshot from 2023-12-17 13-58-39

@alankritdabral
Copy link
Copy Markdown
Author

@koraa can you tell me if this approach is correct

@alankritdabral
Copy link
Copy Markdown
Author

@koraa

Copy link
Copy Markdown

@wesleymatosdev wesleymatosdev left a comment

Choose a reason for hiding this comment

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

IMO it also needs to be checked if the project will actually use prettier or if it'll use other tooling to handle it.

I know you took inspiration from tailcallhq/tailcall, but every project has different needs and we could have checked with @koraa if this is the desired approach before working on the implementation.

Comment thread .github/workflows/ci.yml Outdated
@koraa
Copy link
Copy Markdown
Member

koraa commented Nov 26, 2023

/tip 10

@algora-pbc
Copy link
Copy Markdown

algora-pbc bot commented Nov 26, 2023

@algora-pbc
Copy link
Copy Markdown

algora-pbc bot commented Nov 26, 2023

🎉🎈 @alankritdabral has been awarded $10! 🎈🎊

@alankritdabral
Copy link
Copy Markdown
Author

/tip 10

Thank you for the tip. Should i close the pr as @Min2who wants to get it assigned or should i keep working on it

@koraa
Copy link
Copy Markdown
Member

koraa commented Nov 26, 2023

/tip 10

Thank you for the tip. Should i close the pr as @Min2who wants to get it assigned or should i keep working on it

You are invited to keep working on this or collaborate with @ologbonowiwi or @Min2who or both and if the bounty is low or all three of you want to share in the bounty, I can raise it a bit too (in this case you would have to post a bid in this issue); I am sure either could help you figure out how to tackle this issue.

I'd rather enable people to work on issues than get bogged down by assigning ownership :)

The basic two steps to (1) format examples in the readme and (2) format examples in doc comments can be tackled in parallel anyway!

@koraa
Copy link
Copy Markdown
Member

koraa commented Dec 1, 2023

@alankritdabral Do you intend to finish this project?

@alankritdabral
Copy link
Copy Markdown
Author

@alankritdabral Do you intend to finish this project?
Well i have one approach in mind will be pushing some commits tomorrow. Sorry for the delay 🙏🏻😔😔.

@alankritdabral
Copy link
Copy Markdown
Author

@koraa can you take a look in the code changes you can even run locally

Comment thread format_rust_code.sh Outdated
@wucke13
Copy link
Copy Markdown
Contributor

wucke13 commented Dec 3, 2023

The approach doesn't yet work (but it can be fixed!).

  1. If there is a formatting error, please make sure that the CI job fails. Best case you set an internal variable, but keep checking all files, and only after all files have been checked return the 0 if no error was found or 1 if at least one file violates the formatting.
  2. Some rust doc code directly starts with let var = .... This is detected as a syntax error. You have to enclose this into a fn my_fn(){ } or something similar to make sure that you don't get syntax errors from rustfmt.

@alankritdabral
Copy link
Copy Markdown
Author

@wucke13 what to do with non-utf8 codes rustfmt cant deal with it should i just ignore non-utf8 ??

Copy link
Copy Markdown

@wesleymatosdev wesleymatosdev left a comment

Choose a reason for hiding this comment

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

I don't think the optimal solution is to format the files on CI, as we would need to commit the files

Maybe we can have the script to format the files (as/when users wish), and on CI we fail a pipeline if formatting is not correct

Comment thread .github/workflows/qc.yaml Outdated
@alankritdabral
Copy link
Copy Markdown
Author

I don't think the optimal solution is to format the files on CI, as we would need to commit the files

Maybe we can have the script to format the files (as/when users wish), and on CI we fail a pipeline if formatting is not correct

yes i am going to use rustfmt--check in the ci by default

@wucke13
Copy link
Copy Markdown
Contributor

wucke13 commented Dec 4, 2023

To be clear, the CI shall only check/verify, but it shall not change code on its own.

@alankritdabral
Copy link
Copy Markdown
Author

@wucke13 can you run the workflow and review it
if want to run it locally you can run bash format_rust_code.sh --mode fix

@alankritdabral
Copy link
Copy Markdown
Author

@wucke13 🤓

@koraa
Copy link
Copy Markdown
Member

koraa commented Dec 10, 2023

Hi @alankritdabral,

this is pretty hacky…but also pretty cool. It only solves half of the issue – i.e. formatting examples in markdown code while disregarding markdown code in rustdoc but be that as it may, I am happy to merge this and award the bounty for just that improvement.

If I run --fix and then --check, check still reports issues, can you fix that?

@wucke13 what to do with non-utf8 codes rustfmt cant deal with it should i just ignore non-utf8 ??

Can you elaborate on what you mean by non-utf8? All our source files should be utf8-encoded.

@alankritdabral
Copy link
Copy Markdown
Author

If I run --fix and then --check, check still reports issues, can you fix that?

i have fixed this issue will be pushing in a new pr

@alankritdabral
Copy link
Copy Markdown
Author

alankritdabral commented Dec 11, 2023

Can you elaborate on what you mean by non-utf8? All our source files should be utf8-encoded.

well it was a mistake from my side thee code was treating anything inside fence code with \ as a escape sequence so it was giving an error as non valid utf code i am almost done with this issue will be pushing in a new pr

@alankritdabral
Copy link
Copy Markdown
Author

alankritdabral commented Dec 12, 2023

@koraa i have tested the file now its working fine

@alankritdabral
Copy link
Copy Markdown
Author

@koraa

AlooDon and others added 2 commits December 22, 2023 17:44
This script makes it possible to check formatting of rust code found in the various markdown files in the repo. It is also added as a job to the QC CI workflow.
This applies the novel format_rustcode.sh script to the markdown files in the
repo, to maintain a consistent style across code examples.
@wucke13 wucke13 force-pushed the Enforce-code-style-on-doc-comments-#156 branch from af90515 to ea32bbd Compare December 22, 2023 16:46
@wucke13
Copy link
Copy Markdown
Contributor

wucke13 commented Dec 22, 2023

@alankritdabral I took the liberty to squash your commits and apply the tool as well. Thank you for your contribution!

@alankritdabral
Copy link
Copy Markdown
Author

@alankritdabral I took the liberty to squash your commits and apply the tool as well. Thank you for your contribution!

just wanted to know if there is any community for the rosenpass for eg ddiscord?

@wucke13 wucke13 merged commit 7c83e24 into rosenpass:main Dec 22, 2023
@koraa
Copy link
Copy Markdown
Member

koraa commented Jan 16, 2024

@alankritdabral I took the liberty to squash your commits and apply the tool as well. Thank you for your contribution!

just wanted to know if there is any community for the rosenpass for eg ddiscord?

Yes there is, you would be most welcome to join our matrix developer chat :)

https://matrix.to/#/#rosenpass-dev:matrix.org

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce code style on doc comments

5 participants