DASSL / Gradebook

Open-source product to provide a practical means for instructors to record student attendance and assessment
Other
8 stars 4 forks source link

Add prepareDB.psql, addReferenceData.sql, and README #54

Closed smurthys closed 7 years ago

smurthys commented 7 years ago

The changes in this PR add a script to automate the creation of various database objects.

Here is a summary of the changes:

@afig: I have added the INSERT statement necessary to add reference attendance statuses to addReferenceData.sql

afig commented 7 years ago

Changes look good and the script works well.

Minor comment fix:

One thing to consider is that similar to the situation with the Attendance import, the current working directory must be the same as the location of the current script, or else the prepareDB.psql script will not be able to locate any of the other scripts. It might be worth mentioning that requirement in the README and possibly inside of the prepareDB.psql file.

smurthys commented 7 years ago

Thanks @afig for pointing out the issues.

Commit f10f91e updates comments and revises the README.

BTW, I forgot to mention earlier that prepareDB.sql is based on a script with the same name in ClassDB.

smurthys commented 7 years ago

@afig Thanks for the pointer on file names in the PR title. BTW, Do you have a link where I can read a bit more about your concern and best practices?

afig commented 7 years ago

I am afraid not as far as PR titles or merge commit messages are concerned. After re-analyzing, it seems like changing just the PR title should be sufficient.

I mainly brought it up because the current title of the PR, "Add prepareDB.psql", does not seem to encompass all changes in this PR, including the changes to the reference data. This might create confusion when looking back at the history of the master branch.

GitHub seems to have a specific default format for the message of a merge commit:

Merge Pull Request <##> from <repository owner>/<branch-name>

<PR title>

Both the subject (first line) and body (remaining lines) of the merge commit can be changed, but the subject should not be changed for consistency with other merge commits.

If there is only one commit in the PR, GitHub seems to automatically set the subject of that first commit the title of the PR, which creates issues if later commits in the PR make substantial changes, or if the subject of the first commit did not completely summarize the changes made. Sometimes this issue is corrected if the branch name summarizes the changes well enough, but that does not seem to be the case in this branch.

Overall, it is not a big deal, but makes it easier to determine when certain changes were made when looking at only the merges that have been performed.

smurthys commented 7 years ago

Thanks @afig for the clarification. I changed the PR title to list the files added.