Skip to content

Start to add lots of documentation #899

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

Merged
merged 3 commits into from
Aug 2, 2017

Conversation

carols10cents
Copy link
Member

I started this a while ago, but an email from @vignesh-sankaran prompted me to get what I have out there rather than bitrotting on my machine.

I'd love a review of what I have so far, @vignesh-sankaran! What do you think of the outline of the architecture document? What kinds of things would make that document a more useful high-level overview?

The doc comments should hopefully make the docs at https://docs.rs/cargo-registry/ better, I don't think the little bit I've added here warrants a version bump and publish yet, though.

@jtgeibel
Copy link
Member

Hey @carols10cents, here are my cleaned up notes from going through the layout of my working directory. I have this as docs/REPO_LAYOUT.md for now.

Repo File Layout

Backend

  • build.rs - Cargo build script
  • Cargo.lock - Locks dependencies to specific versions providing consistency across development and deployment
  • Cargo.toml - Defines the crate and its dependencies
  • migrations/ - Diesel migrations applied to the database during development and deployment
  • .rustfmt.toml - Defines Rust coding style guidelines which are enforced by the CI environment
  • src/ - The backend's source code
  • target/ - Compiled output, including dependencies and final binary artifacts - (ignored in .gitignore)
  • tmp/index-co - The registry repository; in production this is cloned from Github and in development from tmp/index-bare - (ignored in .gitignore)

Front End

  • app/ - The frontend's source code
  • config/{environment,targets}.js - Configuration of the frontend
  • dist/ - Contains the distributable (optimized and self-contained) output of building the frontend; served under the root / url - (ignored in .gitignore)
  • .ember-cli - Settings for the ember command line interface
  • ember-cli-build.js - Contains the build specification for Broccoli
  • .eslintrc.js - Defines Javascript coding style guidelines (enforced during CI???)
  • mirage/ - A mock backend used during development and testing
  • node_modules/ - npm dependencies - (ignored in .gitignore)
  • package.json - Defines the npm package and its dependencies
  • package-lock.json - Locks dependencies to specific versions providing consistency across development and deployment
  • public/ - Static files that are merged into dist/ during build
  • testem.js - Integration with Test'em Scripts
  • tests/ - Frontend tests
  • vendor/ - frontend dependencies not distributed by npm; not currently used

Deployment - Heroku

  • app.json - Configuration for Heroku Deploy Button
  • .buildpacks - A list of buildpacks used during deployment
  • config/nginx.conf.erb - Template used by the nginx buildpack
  • .diesel_version - Used by diesel buildpack to install a specific version of Diesel CLI during deployment
  • Procfile - Contains process type declarations for Heroku

Development

  • docs/ - The location of design documentation
  • .editorconfig - Coding style definitions supported by some IDEs // TODO: Reference extensions for common editors
  • .env - Environment variables loaded by the backend - (ignored in .gitignore)
  • .env.sample - Example environment file checked into the repository
  • .git/ - The git repository; not available in all deployments (e.g. Heroku)
  • .gitignore - Configures git to ignore certain files and folders
  • script/init-local-index.sh - Creates registry repositories used during development
  • tmp/ - Temporary files created during development; when deployed on Heroku this is the only writable directory - (ignored in .gitignore)
  • tmp/index-bare - A bare git repository, used as the origin for tmp/index-co during development - (ignored in .gitignore)
  • .travis.yml - Configuration for continous integration at https://travis-ci.org/rust-lang/crates.io
  • .watchmanconfig - Use by Ember CLI to efficiently watch for file changes if you install watchman

Other

  • LICENSE-APACHE
  • LICENSE-MIT
  • README.md

Copy link
Contributor

@vignesh-sankaran vignesh-sankaran left a comment

Choose a reason for hiding this comment

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

Hmm... could we look at removing "The" in front of the headers in docs/ARCHITECTURE.md that refer to backend modules? There's a lot of them :)


#### The `utils` module

### Code having to do with managing a registry of crates
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not quite sure where you're referring to with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why you're not sure :) What are the possible interpretations that you're deciding between? Note that this has an h3, and the h4s from line 102 to 126 are under this heading, does that help? What would be a better heading?

Copy link
Contributor

@vignesh-sankaran vignesh-sankaran Jul 27, 2017

Choose a reason for hiding this comment

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

I was trying to figure out how this fit into one module, yes I didn't see the h3 for it compared to the h4's that refer to specific modules :)

@vignesh-sankaran
Copy link
Contributor

vignesh-sankaran commented Jul 25, 2017

Thanks for starting on this @carols10cents, the structure of the document itself looks good, though I think we should mention that it's specifically for the backend. Maybe we could look into having a similar document for the front end too?

Also, maybe we could look at adding a diagram of the database schema?

@carols10cents
Copy link
Member Author

though I think we should mention that it's specifically for the backend. Maybe we could look into
having a similar document for the front end too?

Yeah, I have a heading in there for backend and I think I just got tired before making a corresponding frontend heading :)

Hmm... could we look at removing "The" in front of the headers in docs/ARCHITECTURE.md that refer to backend modules? There's a lot of them :)

🤷‍♀️ That's how I'd talk about them though... "Where's the code to do x?" "it's in the db module"

Also, maybe we could look at adding a diagram of the database schema?

I'd be into that, if there's a tool we can use to automate creating the diagram and if we keep it up to date :) Do you know of a good one?

@carols10cents carols10cents force-pushed the document-architecture branch from 6c831d6 to 380704b Compare July 26, 2017 23:45
@carols10cents carols10cents force-pushed the document-architecture branch from 380704b to 4c43811 Compare July 26, 2017 23:49
@carols10cents
Copy link
Member Author

Ok -- I've incorporated @jtgeibel's repo layout docs and took @vignesh-sankaran's suggestion to break this out into separate backend and frontend documents-- I could see this getting long :)

WDYT?


[conduit]: https://crates.io/crates/conduit

These files have to do with the backend:
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "these files and directories"?


Documentation about the codebase appears in these locations:

* `LICENSE-APACHE` and `LICENSE-MIT` - the terms under which this codebase is licensed.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like package.json only lists MIT. I'm not sure if that is an oversight or if we need to word this more carefully. We should probably explicitly call out dual licensing in README.md

Also see: https://docs.npmjs.com/files/package.json#license

bors-voyager bot added a commit that referenced this pull request Jul 29, 2017
920: Doc comments on new pull requests r=carols10cents

Along the lines of #899, we should start mandating doc comments on all new types and functions, and encourage doc comments on modified types. 

Is there a way to check for this in TravisCI as well?
bors-voyager bot added a commit that referenced this pull request Jul 30, 2017
920: Doc comments on new pull requests r=carols10cents

Along the lines of #899, we should start mandating doc comments on all new types and functions, and encourage doc comments on modified types. 

Is there a way to check for this in TravisCI as well?
@carols10cents
Copy link
Member Author

I've addressed the two things yinz pointed out, and in the interest of getting this merged so that we can all start working on this, and because this is all just docs, I'm going to merge my own PR 😎

bors r+

bors-voyager bot added a commit that referenced this pull request Aug 2, 2017
899: Start to add lots of documentation r=carols10cents

I started this a while ago, but an email from @vignesh-sankaran prompted me to get what I have out there rather than bitrotting on my machine.

I'd love a review of what I have so far, @vignesh-sankaran! What do you think of the outline of the architecture document? What kinds of things would make that document a more useful high-level overview? 

The doc comments should hopefully make the docs at https://docs.rs/cargo-registry/ better, I don't think the little bit I've added here warrants a version bump and publish yet, though.
@bors-voyager
Copy link
Contributor

bors-voyager bot commented Aug 2, 2017

Build succeeded

@bors-voyager bors-voyager bot merged commit 25383e5 into rust-lang:master Aug 2, 2017
@carols10cents carols10cents deleted the document-architecture branch August 2, 2017 14:06
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.

4 participants