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

Source directory reorganization #164

Closed afig closed 6 years ago

afig commented 6 years ago

This PR separates all script files into one of three kinds: core, recommended, and optional (core, reco, and opt directories, respectively). Additionally, script files are now also separated into database scripts and server scripts (db and server directories under src, respectively).

Suffixes were added to file names corresponding to which of the three kinds they belong to.

For more information, see issue #142, which this PR fixes.

In terms of review, ensure that files are properly categorized. Particularly addCatalogMgmtOpt.sql, as I do not recall whether it was decided if it was recommended or optional.

Also, I left the uninstallation scripts (removeFromDB.sql and removeFromServer.sql) uncategorized (outside of a core, reco, or opt directory), as they remove all portions of the system.

I updated the file names inside of the prepareDBCore.psql script, added addUserMgmtCore.sql and addClassDBRoleViewsCore.sql, and removed addConnectionMgmt.sql.

wildtayne commented 6 years ago

Looks good @afig. I think all the scripts have the correct placement. IMO, based on our discussion, addCatalogMgmtOpt.sql should be Opt.

The script names still need to be changed inside each file, but I imagine this is best left until we are sure of the location of each script. Also, should we include the 'full' path (ie. db/core/addXYZCore.sql), or just the filename in the prologue comments?

Edit: Maybe there should be a separate uninstall or similar folder under db and server. I say this because having disableConnectionLoggingReco.psql in the Reco folder, but the other removal scripts not in a dir seems to imply that disableConnectionLoggingReco.psql is an install script. The folder would give as a place to put other uninstall scripts way may write in the future, as well. An alternate solution would be to just put disableConnectionLoggingReco.psql in root of server.

Edit Edit: Either way, this should be merged before the placement of disableConnectionLoggingReco.psql is resolved, since that will not affect work on other scripts.

afig commented 6 years ago

Thanks for pointing out that the file names have to be updated for the headers, I had overlooked that when making these changes. However, I think it might over-complicate things if we add the full path.

I agree that the currently placement of disableConnectionLoggingReco.psql is rather confusing, but then again, it's not really an "uninstall" script either... 😕. I think it will be best to leave it for now and discuss it at some other point.

smurthys commented 6 years ago

Excellent work and analysis @afig and @srrollo.

I like the current location of the remove*.sql files. I recommend renaming them to removeAllFrom*.sql so it is obvious they will remove everything.

smurthys commented 6 years ago

I recommend renaming the removeFrom*.sql files as part of this PR to avoid issue-management effort. However, I am OK if the renaming is done after due to other considerations.

afig commented 6 years ago

The two files were renamed and headers updated:

removeFromDB.sql -> removeAllFromDB.sql

removeFromServer.sql -> removeAllFromServer.sql

smurthys commented 6 years ago

Nicely done. I'm eager to see how the new directory structure plays out.