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

Created table of contents, corrected spelling and punctuation #96

Merged
merged 8 commits into from
May 19, 2023

Conversation

OlaPom
Copy link
Contributor

@OlaPom OlaPom commented Apr 24, 2023

  • corrected minor stylistic and spelling errors
  • added section numbers
  • created a table of contents

@OlaPom
Copy link
Contributor Author

OlaPom commented Apr 24, 2023

Issue #79

@amtoine amtoine linked an issue Apr 24, 2023 that may be closed by this pull request
@amtoine
Copy link
Member

amtoine commented Apr 24, 2023

Issue #79

you can use https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue directly in the PR description if you want to link it yourself 😉
e.g. should fix #79 😋

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

simple and great, looks good to me 👌

README.md Show resolved Hide resolved
@ctmbl ctmbl added documentation Improvements or additions to documentation Priority: Low The Issue will be address only if other tasks are done labels Apr 24, 2023
@ctmbl ctmbl requested review from atxr and ctmbl April 24, 2023 21:01
Copy link
Contributor

@ctmbl ctmbl left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution! I added a few suggestion I already had in mind 😉

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@OlaPom OlaPom requested review from ctmbl and amtoine May 4, 2023 11:15
ctmbl
ctmbl previously approved these changes May 4, 2023
Copy link
Contributor

@ctmbl ctmbl left a comment

Choose a reason for hiding this comment

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

Good to go!

@amtoine
Copy link
Member

amtoine commented May 4, 2023

@OlaPom
that looks good but the CI is not happy 😏
you will have to run the same prettier command as in the CI to fix the formatting 😌

@OlaPom
Copy link
Contributor Author

OlaPom commented May 12, 2023

I've been looking for instructions, but I need an explanation. Can this be done with GitHub Actions or do I need to install some tools? (Because if it's the latter, I'm afraid my technical skills may not be enough).

@amtoine
Copy link
Member

amtoine commented May 12, 2023

@OlaPom

I've been looking for instructions, but I need an explanation.

this should be added in the repo 👍

Can this be done with GitHub Actions or do I need to install some tools? (Because if it's the latter, I'm afraid my technical skills may not be enough).

yeah we chose not to commit in the name of our contributors, to control what's going on in the PR branches 😌
i'm not a node nor JS chad, but here is a command that does the job

npx prettier --write .

Note
see https://prettier.io/docs/en/install.html

@ctmbl
Copy link
Contributor

ctmbl commented May 12, 2023

@OlaPom

I've been looking for instructions, but I need an explanation. Can this be done with GitHub Actions or do I need to install some tools? (Because if it's the latter, I'm afraid my technical skills may not be enough).

This can't be done with GitHub Actions, because we don't want automated tools to modify our code without our approbation.
However this shouldn't be a problem for this doesn't require any technical skills.

First you need to install npm
Then in a terminal (on Windows: Windows Key + R then type "cmd.exe") run npm install -g prettier
Finally at the source folder of this repo run prettier --write README.md
This should do!
If you still believe you can't do it, I will no problem 😉

PS: to remove it afterwards (if you want to), run npm uninstall -g prettier

@ctmbl
Copy link
Contributor

ctmbl commented May 12, 2023

@OlaPom
Also there are conflicts with the main branch, you should solve it with a git merge not a git rebase
Again if you want I can do it 😉

@amtoine
Copy link
Member

amtoine commented May 12, 2023

@ctmbl jinx 😏

@ctmbl
Copy link
Contributor

ctmbl commented May 15, 2023

@OlaPom after your git merge main you'll have the recent #99 on your dev branch, so you'll be able to just do npm run pretty to prettify the code!

Note
you'll still need to install prettify first

@OlaPom
Copy link
Contributor Author

OlaPom commented May 16, 2023

Thank you for the instructions! Let me try 💪

@ctmbl
Copy link
Contributor

ctmbl commented May 16, 2023

No problem, looking forward to see your commit!

atxr
atxr previously approved these changes May 16, 2023
Copy link
Contributor

@atxr atxr 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 you contribution and for correcting my (stupid) English typos 🙂
You did a good job 🥳
We just wait for prettier now, and then we will merge your work 🥇
(don't hesitate to reach us if you experience any trouble with prettier)

@OlaPom
Copy link
Contributor Author

OlaPom commented May 17, 2023

I installed prettier, and it works, but the rule doesn’t. When I do npm run pretty in the main folder of the repo, I get this:

iscsc.fr@0.1.2 pretty

npx prettier --write ‘/*.{js,md,yml,css,html}’ ‘!mongodb/

[error] No files matching the pattern were found: “‘**/*.{js,md,yml,css,html}’”.

[error] No files matching the pattern were found: “‘!mongodb/**’”.

I’m on Windows. I googled this error, and it may have sth to do with the quotes.

I copied the readme file to another folder and was able to format it with prettier --write. I tried it on a copy for now, because I thought you’d prefer people to run the script. Let me know if it makes any difference. If not, I’ll format the file and push it. Or let me know if I need to do anything to make the rule work on my computer.

@atxr
Copy link
Contributor

atxr commented May 17, 2023

@OlaPom I ran prettier on my machine and created a pull request on your branch
You can merge it and it will resolve this PR
Sorry for the disturbances

@ctmbl
Copy link
Contributor

ctmbl commented May 17, 2023

Like @atxr I managed to run it on Linux, sorry it doesn't work on Windows, but tks for testing it at least we'll be able to solve it now 🥲

Once you'll have merged atxr's PR on your repo we'll merge this one, in any case, tks for your contribution!

@OlaPom OlaPom dismissed stale reviews from atxr and ctmbl via c318e17 May 18, 2023 08:20
@OlaPom
Copy link
Contributor Author

OlaPom commented May 18, 2023

Ok. I think it's done. Thanks for help!

@amtoine
Copy link
Member

amtoine commented May 18, 2023

CI did pass 🥳

@atxr atxr requested review from ctmbl and atxr May 19, 2023 09:11
Copy link
Contributor

@atxr atxr left a comment

Choose a reason for hiding this comment

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

Thanks again for contributing! 💯

@atxr atxr merged commit cc17152 into iScsc:main May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Priority: Low The Issue will be address only if other tasks are done
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

make the README easier to read
4 participants