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

feat: adapt to rst #301

Merged
merged 12 commits into from
Sep 27, 2022
Merged

feat: adapt to rst #301

merged 12 commits into from
Sep 27, 2022

Conversation

12rambau
Copy link
Contributor

Adapt the indentation level of what's written to the initial indent of the opening tag. It should change nothing to markdown input.

What:
The table and badge created by the CLI are raw HTML. They cannot be used in .rst fie because RST files are sensible to indentation. To be compatible the tool should get the indentation of the first tag and apply it to the rest of the content (as a global offset). #300

Why:
To make the tool compatible with .rst file => every doc created with Sphinx

How:
I'm getting the index of the last \n before the start Tag and I then replace every \nof the newcontent with an appropriate number of extra spaces.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

Comments
I don't want/can add nodes dependencies to my project (that's why I was interested by the bot) so I can't test this functionality myself. I've authorized the edit by maintainer but let me know if I need to change things on my side.

Adapt the indentation level of what's written to the initial indent of the opening tag. It should change nothing to markdown input.
Copy link
Member

@Berkmann18 Berkmann18 left a comment

Choose a reason for hiding this comment

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

I tried pushing changes but it blocked me so I'll just write the changes as suggestions.

Copy link
Member

@Berkmann18 Berkmann18 left a comment

Choose a reason for hiding this comment

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

The tests need to be updated as well (one of the updates would require a local command, unless you fancy manually updating snapshots):
image

src/generate/index.js Outdated Show resolved Hide resolved
src/generate/index.js Outdated Show resolved Hide resolved
src/generate/index.js Outdated Show resolved Hide resolved
src/generate/index.js Outdated Show resolved Hide resolved
@Berkmann18 Berkmann18 self-assigned this Jun 26, 2021
@Berkmann18 Berkmann18 linked an issue Jun 26, 2021 that may be closed by this pull request
@12rambau
Copy link
Contributor Author

I tried pushing changes but it blocked me so I'll just write the changes as suggestions.

this is weird as I set this:

Capture d’écran 2021-06-29 à 11 52 37

boh it will just be slowlyer from my side, I'll review them AFAP

12rambau and others added 4 commits June 29, 2021 11:54
Co-authored-by: Maximilian Berkmann <maxieberkmann@gmail.com>
Co-authored-by: Maximilian Berkmann <maxieberkmann@gmail.com>
Co-authored-by: Maximilian Berkmann <maxieberkmann@gmail.com>
Co-authored-by: Maximilian Berkmann <maxieberkmann@gmail.com>
@12rambau
Copy link
Contributor Author

12rambau commented Jun 29, 2021

according to the image you send it seems that something is not running the way I would like, I'm trying to install the yarn project locally but I can't manage to make it work.

I've cloned my brnach on my local computer and run : yarn init in my fake project. Then to install the lib I ran: yarn add file:/Users/pierrickrambaud/Documents/travail/FAO/app_buffer/all-contributors-cli which seems to work as well.

When I check node_modules/.bin/, all-contributors is nowhere to be found, is it normal ?

@Berkmann18
Copy link
Member

Hmm, if you wanted to install the cli you should have done either:

npm i all-contributors-cli

or

yard add all-contributors-cli

Well, assuming you wanted to install it in a project and not as a dev/prod dependency.

@12rambau
Copy link
Contributor Author

12rambau commented Sep 2, 2021

Sorry for the very late answer but i had no time to dedicate to this PR these past month.
So as I explained I would like to test the modifications I made on a fake project but i don't understand how node_module works.

how can I test the cli in "dev" mode ?

@Berkmann18
Copy link
Member

Berkmann18 commented Nov 29, 2021

@12rambau That's fine, I was also very busy with other stuff.
The node_modules contain packages installed by NPM with npm i, to test the cli you'll use npm t to see if tests pass and npm start to see it running locally.

@12rambau
Copy link
Contributor Author

12rambau commented Dec 5, 2021

i ran npm i which seemed to worked (I'll ignore deprecation warnings) and then npm t that led to error in the snapshots as you mentioned.

Now if I want to use it (the local version from my computer) in a fake folder (to check waht happens) what should I do ?
If I run npm start from the cli folder, I get the following error:

sh: ./dist/cli.js: No such file or directory

And obviously the all-contributor cli is not available.
I'm srry if you think that my questions are trivial but there is not contributing section in the doc. i'll be happy to create one onc I finally mange to run it once

@Berkmann18
Copy link
Member

If I run npm start from the cli folder, I get the following error:

Have you tried running it from the root of the repo?

@12rambau
Copy link
Contributor Author

I get the following error :

> all-contributors-cli@0.0.0-semantically-released start /Users/pierrickrambaud/Documents/travail/FAO/app_buffer/all-contributors-cli
> ./dist/cli.js

sh: ./dist/cli.js: No such file or directory
npm ERR! code ELIFECYCLE
npm ERR! syscall spawn
npm ERR! file sh
npm ERR! errno ENOENT
npm ERR! all-contributors-cli@0.0.0-semantically-released start: `./dist/cli.js`
npm ERR! spawn ENOENT
npm ERR!
npm ERR! Failed at the all-contributors-cli@0.0.0-semantically-released start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/pierrickrambaud/.npm/_logs/2022-01-12T07_55_41_479Z-debug.log

The use case is trivial, I've pulled the repository to my local computer, created a fake repository with nothing in it but a README.md file. I just want to start the CLI locally (using my branch and not the distant package) and check the output of the commands in this very repository. Once done I'll be able to modify the snapshot and finish this PR.

I'm sure it's 101 developing procedure. can you write down the steps to reproduce it from my end ?

@tenshiAMD tenshiAMD self-assigned this Sep 22, 2022
@tenshiAMD
Copy link
Member

@12rambau I've fixed the checks. do you mind adding some tests? Thanks.

@tenshiAMD tenshiAMD requested review from gr2m and a team and removed request for gr2m September 27, 2022 15:30
@12rambau
Copy link
Contributor Author

first thank you very much for taking over the review of this PR !
I would be glad to help but as long as I don't understand how to start it locally I won't be able to do anything. If you look at my previous exchange you will realize that I barely understand how I'm supposed to use this tool locally (I'm actually interested by the bot in the first place). I let you decide if it's more time-consuming for you to explain to me or to write the tests yourself.

@tenshiAMD
Copy link
Member

@Berkmann18 I tested both .md and .rst are working

@tenshiAMD
Copy link
Member

@12rambau just install the latest version after we merged this one. can you verify if this one works well when you used it on your end?

@Berkmann18 merging this one since this does not break the other tests.

@tenshiAMD tenshiAMD enabled auto-merge (squash) September 27, 2022 16:49
@tenshiAMD
Copy link
Member

@all-contributors please add @12rambau for code

@allcontributors
Copy link
Contributor

@tenshiAMD

I've put up a pull request to add @12rambau! 🎉

@tenshiAMD tenshiAMD dismissed Berkmann18’s stale review September 27, 2022 17:06

merging this one since this does not break the other tests.

@tenshiAMD tenshiAMD merged commit 7a9150f into all-contributors:master Sep 27, 2022
@all-contributors-release-bot
Copy link
Member

🎉 This PR is included in version 6.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@12rambau
Copy link
Contributor Author

12rambau commented Sep 28, 2022

I still get the same errors as I was mentioning earlier in the thread, it may be related to my machine but I don't have the bandwidth to do a course on NPM packaging. I'll trust you on this one, as long as the table is indented relative to the code-block that's perfect.

I'll wait until it's available via the bot to test it in my Python projects

@tenshiAMD
Copy link
Member

I'll wait until it's available via the bot to test it in my Python projects

@12rambau you can try now

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.

make the bot work with rst files
4 participants