DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
8 stars 2 forks source link

Installer NOTICE Suppression and Uninstaller Improvements #90

Closed wildtayne closed 7 years ago

wildtayne commented 7 years ago

This branch contains fixes in two areas:

NOTICE Suppression:

Uninstaller Improvements:

Misc:

afig commented 7 years ago

We will likely want to rename "Schemas for CS205" in the first line of any Shelter files. Perhaps something like "Example Schemas for ClassDB"

The NOTICE suppression looks good, however, it might also suppress other NOTICEs. Not ones that we explicitly raise, but rather other NOTICEs that PostgreSQL might raise under certain conditions. This does not seem like it would be a major issue, but just something to keep in mind.

Otherwise the changes look good.

smurthys commented 7 years ago

I agree with @afig on 'Schemas for CS205' and take his cue to recommend caution on suppressing notices.

Could a SET LOCAL client_min_messages TO WARNING; be helpful in removeClassDBFromServer.sql?

There is a 'TBD' comment in removeClassDBFromDB.sql leftover from the initial outline of the script.

All see this comment I just made on the change to removeClassDBFromDB.sql.

wildtayne commented 7 years ago

Thanks for the feedback. I do think removeClassDBFromServer.sql could use NOTICE suppression, however, I am starting to think about some of the drawbacks. For example, if somehow ClassDB got into an inconsistent state where, say, two of the four core roles were missing, it would be helpful to get that feedback from the script. I still think it is an OK solution, but past M1 we may want to come up with a nicer one. One idea off the top of my head would be to create our own DROP IF EXISTS function that has nicer output, or keep notices suppressed, but output a summary of how many objects were created/dropped/already existed at the end of the script.

I've pushed some new commits that:

smurthys commented 7 years ago

I just noticed that removeClassDBFromDB.sql uses the same essential query twice in reassigning schemas. It is better to use just the SELECT query presently inside the EXECUTE. For readability, put the result of that SELECT query into a VARCHAR or TEXT variable and then EXECUTE the variable after testing it for NULL (or unconditionally execute in combination with a COALESCE).

Also, I think the RAISE NOTICE in that area of the code could be RAISE INFO which has a lower priority level and that level is sufficient here.

wildtayne commented 7 years ago

Good catch, I've made the change. I also added the notice suppression to removeClassDBFromDB.sql, as it also has several IF EXISTS statements. Since we have some INFO and NOTICE messages we do want to display, I've slightly reorganized the file:

I've also ensured that removeClassDBFromServer.sql has the RESET statement so our closing NOTICE is shown.