1601 default vals

[tobz619]

I've figured out the pull request:

I've got quite a few to go but hopefully the strategy I'm going for is apparent and is helpful to you.

If you have any thoughts on some other things or style comments then let me know and I'll try and incorporate them. I'll do the rest of these tomorrow

[mpscholten]

Thanks 👍 Good job so far!

[mpscholten]

We're getting closer :)

[mpscholten]

Typically IHP code is indentend a bit different, like this:

Suggested change

	defColumn = Column {
	name = ""
	, columnType = PUUID
	, defaultValue = Nothing
	, notNull = False
	, isUnique = False
	, generator = Nothing
	}
	defColumn =
	Column
	{ name = ""
	, columnType = PUUID
	-- ....
	}

[mpscholten]

The variables here should be spelled out, e.g. tableName instead of t

Suggested change

	defCreateTable t = CreateTable {
	defCreateTable tableName = CreateTable {

[mpscholten]

IMO this can be just done via defCreateTable. When you have a table that has no columns, you could still pass an empty list like defCreateTable "myTableWithoutCols" [].

[tobz619]

Yes, I agree now, especially now I see how often each pattern appears, it makes more sense to define this in these ways - will alter :)

[mpscholten]

Functions below this lines are IMO not a good idea. By having these very specific functions outside of the test suites we actually make tests harder to change, because you now need to look at two files to understands what's going on. So it would be better to undo the changes below and just make use of defTable and defColumn inside the tests.

[tobz619]

So my thinking here was that if you had repeating units or tests, you can define them once, remember them and then use them as normal and so long as you know the definition of the table or column, you could:

1. reuse it
2. use it as a new base for a table/column

Perhaps it's not useful for whole tables but certainly default values/columns that may repeat and have complicated structures.

That said, I do see your point and there are several items that are superfluous; secondly, I'm sure if you needed to define repeatable units, you would define them as needed and perhaps on the test page like was done before.

Therefore, I will revert the tests to defintions of defColumn etc and leave space for IHP to define its own monomers as required.

[tobz619]

I was also thinking that the writer would check the definition of the test by ctrl+clicking on the name and opening up the references window -> but I guess not everyone uses vscode for IDE which is understandable.

[mpscholten]

Here's an example how I imagine this to be working after the PR is merged:

            let statement = StatementCreateTable (
                defCreateTable "array_tests"
                    [ defColumn "pay_by_quarter" (PArray PInt)
                    ]
            )

[tobz619]

Okay should all now be complete :)

[mpscholten]

As this is test related logic it shouldn't live here

[mpscholten]

maybe we can move this into some test module instead

[mpscholten]

This function doesn't seem useful to me, why can't we just write emptyTable { name = .. , columns = .. } directly?

[tobz619]

It's more for speed: it means you don't have to write the record parts over and over again especially if they're the only two values in emptyTable being changed and you could quickly be certain that the only values changed were the name and the columns contained. If other values needed changing, then it's up to the writer to use emptyTable {..} or (defCreateTable tablename columns){..}) -> both options are always available.

Since they were a recurring feature, it made sense to write them this way initially.

[mpscholten]

Same with those functions, I think it's more clear to write:

emptyTable { name = "users", columns = [ colUUID ] }

instead of

defCreateTableWSetCol "users" "id" ...

[tobz619]

Same as before, for speed (not needing to write records) and composability (ability to easily use (.) and ($) to change values or combine them with other things). I do understand the concerns for clarity's sake though.

Since there were quite a few examples of where only certain values in certain fields are changed, if you know what the functions do, you can have a quick idea of what the table or column looks like or have a fast way of defining a more complex table as a base for another table by using base functions.

Again, the option the use the base tables and columns and record syntax is always there and would be functionally identical.

By using a specific function that encapsulates a pattern, I can quickly reason out the structure of the full table if I know what the function does UNLESS I modify it using ( ) and add more records with {..} or putting another function ahead of it which modifies the table further.

It really depends on much you want to represent specific patterns/structures of tests with functions. Also a lot of these functions can end up being helper functions much larger structures; but these will be made when needed I imagine.

[mpscholten]

I'd prefer to also just write emptyColumn { name, columnType = PText }

[tobz619]

It's written this way because setColumn and setColumnN use emptyColumn as precursors and edits them according to their patterns.

By partially applying setColumn/N with PText, one can make the assumption that any str called with colText will always return a column with the name of str, a columnType of PText and notNull value of True -> so for all cases where one needs to generate a column that will look like so, they can do so with one parameter and not mess with records and be reasonably confident that it will create the desired column.

Once again, the option to write with emptyColumn and records is also available.

[tobz619]

I've been a bit busy so haven't been able to rewrite the tests but I have time to do so tomorrow evening :)

[mpscholten]

Great, thanks for the update! Happy to review again later today 👍

[mpscholten]

Is this ready for review again? :)

[tobz619]

Is this ready for review again? :)

Not yet, hopefully by the end of tonight! :)

I must admit my motivation to rewrite to instances of emptyTable and emptyColumn is low but I'll get it done

[tobz619]

I think, I'm done with this here:

Rewriting instances of defaultCreateTablePKID will take a long time and I feel it does its job and I need to move onto other things.