DASSL / ClassDB

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

Use machine-readable version number to determine current server's version #268

Closed smurthys closed 6 years ago

smurthys commented 6 years ago

This PR makes the following changes:

Fixes #263 #267

smurthys commented 6 years ago

I chose to convert current server's version from int to string for two reasons (instead of converting the input string version number to int):

KevinKelly25 commented 6 years ago

Thank you @smurthys for adding this useful function. Everything works well on 9.4 and 10.

The only small thing that I noticed was the use of v as a variable. While it is rather easy to guess the meaning of v, there aren't many places, if any, in ClassDB where it isn't immediately evident what a variable means. With v it took me a few seconds to make sure I knew the meaning. To me it doesn't fit into the nomenclature of ClassDB where it is normally easily readable. It seems like using version instead of v would fit better. However, it might be just me overthinking this and it is worth a small discussion.

afig commented 6 years ago

Looks great, thanks for updating the implementation @smurthys. The functions work as expected and tests pass on versions 9.6 and 10, and converting from int to string is certainly ideal all things considered.

I have two minor suggestions:

I also agree with @KevinKelly25's suggestion about replacing v with a more descriptive variable name.

smurthys commented 6 years ago

Thank you for the review. It seems like I forgot to replace the variable name I had used in the version I had developed in pgAdmin. Thanks for catching that.

I just pushed changes that address the three issues pointed out. Because a parameter name changed, please run the following query before running the revised script. This step is not required in production:

DROP FUNCTION ClassDB.intServerVersionToString(INT);
smurthys commented 6 years ago

Thanks for the reviews.