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

Generate clippy-book - Issue #6011 #6779

Closed
wants to merge 15 commits into from

Conversation

moaz-mokhtar
Copy link

Greetings to all,

This is in reference to solve issue #6011 I did pull first edition of book and kindly note below:

  • Folder which contains mdbook implementation is called clippy-book.
  • I did move *.md from doc/ to clippy-book/src because when it was in doc/, css files references need to edited in each .html manually.
  • I didn't implement CI yet, I will do and update you.
  • Note I tried to test the mdbook but shows error as below:
    image

Seeking your feedback.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 23, 2021
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

NIT about the dir name for the book: Everything else is named clippy_* with an underscore. We should apply the same here.

(On a related note: Maybe we should listen to one of our lints, we have for modules/enums/... and apply it to our dir structure and remove the clippy_ prefix from those.)

Cargo.lock Outdated
@@ -0,0 +1,726 @@
# This file is automatically @generated by Cargo.
Copy link
Member

Choose a reason for hiding this comment

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

Don't add a Cargo.lock

index.html Outdated
@@ -0,0 +1,385 @@
<!DOCTYPE HTML>
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this file in the repo? Isn't this generated by mdbook?

Copy link
Author

Choose a reason for hiding this comment

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

That is the concern that mdbook put .html file in the same related .md file directory. That is why I moved .md files in doc/ to the <book_folder>/src directory.
Another issue for the README.md, the index.html style will not be applied except I did fix manually the css location inside index.html file.

For the index.html I have 2 options:

  • Copy README to the <book_folder>/src directory. So each time README changed, it should be copied to that dir.
  • Keep it and manually fix the css location inside the `index.html'

Normal page inside 'clippy_book/dir` Note html directory.
image

Home page inside main directory Note html directory.
image

Copy link
Member

Choose a reason for hiding this comment

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

I really don't think, that you have to add any css or html files to get the standard formatting of the book, which we want. If I'm mistaken, please link me to the documentation explaining why a seems-to-be-generated html file is necessary in the repo.

Copy link
Author

Choose a reason for hiding this comment

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

Generating css and html done automatically by mdbook build. So I did mdbook clean so the extra (html, css, js) files removed and only main files kept.

clippy-book/book.toml Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Feb 24, 2021

☔ The latest upstream changes (presumably #6756) made this pull request unmergeable. Please resolve the merge conflicts.

@moaz-mokhtar
Copy link
Author

(On a related note: Maybe we should listen to one of our lints, we have for modules/enums/... and apply it to our dir structure and remove the clippy_ prefix from those.)

Apologize... I didn't get the point we have for modules/enums/, I did fix by making a folder named clippy_book.

@moaz-mokhtar
Copy link
Author

@bors
I followed git conflict notes as below but shows access rights error.

rust-clippy on  dev_book [✘!?] is 📦 v0.1.52 via 🦀 v1.52.0-nightly took 23s 
❯ git checkout -f master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

rust-clippy on  master [?] is 📦 v0.1.52 via 🦀 v1.52.0-nightly 
❯ git pull upstream master
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Then tried

rust-clippy on  dev_book [?] is 📦 v0.1.52 via 🦀 v1.52.0-nightly 
❯ git pull upstream master
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
rust-clippy on  master [?] is 📦 v0.1.52 via 🦀 v1.52.0-nightly 
❯ git pull upstream dev_book
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

@llogiq
Copy link
Contributor

llogiq commented Feb 27, 2021

@flip1995 suggested we remove the clippy_ prefix from all subfolders, because if that were a module structure, it would trigger the lint for double prefixes in item names (whose name I just forgot).

Don't worry about it, it's unrelated to this PR.

@moaz-mokhtar
Copy link
Author

@llogiq
I got it. Ok I will do it.

@llogiq
Copy link
Contributor

llogiq commented Mar 3, 2021

What is with the "Don't worry about it, it's" file you seem to add?

@flip1995
Copy link
Member

flip1995 commented Mar 4, 2021

3b02343 should be done in a separate PR, if at all. We haven't discussed this, it was just an idea I had, not something anyone has agreed to. (And we should only do this if there is less traffic/fewer open PRs)

@moaz-mokhtar
Copy link
Author

Very thankful for your support and good way of communication.

Noted, I will return it back.

@moaz-mokhtar
Copy link
Author

I did revert removing prefix clippy_* and accept my apology.

There are 2 points now which I mentioned before.
First: Author name as my feedback link.
Second: 'index.html' file in main directory issue as my feedback link.

clippy_book/book.toml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@moaz-mokhtar
Copy link
Author

Very thankful for your respectful cooperation.

Now book updated with respect to Authors naming standards but still seeking your feedback which have issue in 'index.html' file in main directory as per link.

@flip1995
Copy link
Member

Can you please clean up this PR? There are many generated and unnecessary files pushed. It is almost impossible to review this or give any pointers what to do next.

@flip1995
Copy link
Member

flip1995 commented Mar 11, 2021

There are still some unrelated changes in this PR. Like the Cargo.lock file, the .gitignore and the Chapter 1 file.

As next steps, the book has to be formatted/ordered and a short step-by-step instruction added how to build the book.

@moaz-mokhtar
Copy link
Author

noted

@bors
Copy link
Contributor

bors commented Mar 17, 2021

☔ The latest upstream changes (presumably #6888) made this pull request unmergeable. Please resolve the merge conflicts.

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 17, 2021
@giraffate
Copy link
Contributor

ping from triage @moaz-mokhtar. Can you have any update on this?

@giraffate
Copy link
Contributor

ping from triage @moaz-mokhtar. According to the triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen this.

@giraffate giraffate closed this Apr 20, 2021
@giraffate giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 20, 2021
@moaz-mokhtar
Copy link
Author

Apologize for being late.
I'm going to finalize it.

@moaz-mokhtar
Copy link
Author

Excuse me I'm not able to reopen it which I don't found any reopen button or link! Should I create a new Pull Request?

@giraffate giraffate reopened this Apr 22, 2021
@giraffate giraffate added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-inactive-closed Status: Closed due to inactivity labels Apr 22, 2021
@giraffate
Copy link
Contributor

@moaz-mokhtar Thanks, and no worries! I reopened this.

@moaz-mokhtar
Copy link
Author

Conflicts with master resolved.

I did copy README.md to clippy_book/src/ directory due to issue which mentioned in this link.

@flip1995
Copy link
Member

This PR is still missing many things:

  1. Structure of the book – There is no point of creating a book if it is just all the documentation we have moved to another directory.
  2. Questionable changes – Why was the Readme moved? Why was .gitignore removed?
  3. Build instructions – How to build the book? How can we include it in CI/CD? This does not have to be done in this PR, but we should lay the groundwork for the next steps in this PR
  4. What is the benefit of this? – We want a Clippy book. But the main reason we want it, is to get more structure in our documentation. This PR only moves files around. Should we change the Readme? What information should go where? ...?

Addressing this concerns is probably only possible, if you have some knowledge of how Clippy works internally. Since you're a first-time contributor @moaz-mokhtar, I'm not sure if you can address my concerns just because of the lack of experience. If you have your own concept on how the Clippy Book should look like, feel free to suggest this in this PR and we can work from there to find a common ground. But in the current state someone more experienced with Clippy would have to go over this PR and do the actual hard work of giving the book structure so that it actually improves the documentation situation.

@giraffate
Copy link
Contributor

ping from triage @moaz-mokhtar. Can you have any update on this?

@giraffate
Copy link
Contributor

ping from triage @moaz-mokhtar. According to the triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen this.

@giraffate giraffate closed this May 27, 2021
@giraffate giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants