-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for updating! Added a few minor comments.
README.md
Outdated
``` | ||
mysql -u root -p | ||
mysql> CREATE DATABASE IF NOT EXISTS eregs; | ||
mysql> CREATE DATABASE IF NOT EXISTS test_eregs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be removed as this database is created by Django automatically when unit tests are run.
README.md
Outdated
|
||
### Import a regulation | ||
|
||
The application requires [regulation xml](https://github.com/cfpb/regulations-xml) data. To simplify development, sample data has been provided. To load the sample data into the database run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use term "RegML" instead/in addition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe say, "RegML-compliant xml."
README.md
Outdated
@@ -3,32 +3,95 @@ | |||
[![Build Status](https://travis-ci.org/cfpb/eregs-2.0.svg?branch=master)](https://travis-ci.org/cfpb/eregs-2.0) | |||
[![Coverage Status](https://coveralls.io/repos/github/cfpb/eregs-2.0/badge.svg?branch=master)](https://coveralls.io/github/cfpb/eregs-2.0?branch=master) | |||
|
|||
This is the new version of eRegs. | |||
Work in progress. This is an updated back-end for consumerfinance.gov/eregulations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a full link, e.g. https://www.consumerfinance.gov/eregulations/?
README.md
Outdated
Login to MySQL and create a database named `eregs` with an `eregs` superuser. | ||
|
||
``` | ||
mysql -u root -p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe remove the -u root -p
as that assumes a certain set of login credentials for MySQL (root user has no password) which might not always be true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do need to log in to MySQL somehow, though. I think it's a safe assumption that anyone doing local development and installing MySQL via homebrew has root access to it; if they have a non-blank password, they'll be prompted so I don't think this is a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should ditch the creation of the test database as per Andy's comment and probably say something like "RegML-compliant xml" where noted below. Otherwise I think it's great.
Thanks for the review @chosak and @grapesmoker! I just pushed changes that should address all of your recommendations. |
This fleshes out the README with full installation instructions and a link to the documentation.