First attempt at customising prefixes #776

[olivierdeckers]

I gave #776 a try, and wanted to get some initial feedback.

This implementation doesn't interact very nicely with setting mpsPrefixFields to False. An empty entity name will then be passed to the function, which is surprising and limiting.
Maybe I could instead make the default implementation smarter to get around this?

Before submitting your PR, check that you've:

• Bumped the version number
• Documented new APIs with Haddock markup
• Added @since declarations to the Haddock

After submitting your PR:

• Update the Changelog.md file with a link to your PR
• Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

[parsonsmatt]

Great first pass! We can tidy up the implementation to give a nicer migration path for users currently using mpsPrefixFields = False, though.

Thanks for the PR 😄

[parsonsmatt]

This is a great integration test for the feature! Thanks for writing it up.

[parsonsmatt]

I'd prefer for things to just keep working as expected as much as possible. The corner case that you mention is that someone has mpsPrefixFields = False, and they go to use the mpsFieldLabelModifier. Obviously, they'd expect the entity name passed in with mpsFieldLabelModifier function, and not just empty string.

I'm tempted to say that if they have specified mpsPrefixFields = False, then we just pass both things in to the function. The user is free to have a function like:

mpsFieldLabelModifier = \entity field -> "_schema_x_" <> field

With this design, we don't know if the user has provided an implementation or is using the default. So perhaps we should set the type here to mpsFieldLabelModifier ::Maybe (Text -> Text -> Text) and a default of Nothing.

Then the logic looks like:

case mpsFieldLabelModifier mps of
  Nothing -> do the logic as currently written
  Just fn -> provide both entity name and field to the function

This basically makes mpsPrefixFields redundant, in the case that the user specifies mpsPrefixFields = False, mpsFieldLabelModifier = Just ....

Another thing we'll want to consider: if they have mpsPrefixFields = False, do we want to uppercase the first character of the field name for the user? Whatever choice we make should be documented.

[olivierdeckers]

I've thought about this some more. I make two observations:

1. We want the change to be backwards compatible for users using mpsPrefixFields = False who don't want to use the mpsFieldLabelModifier
2. Other than for this backward compatibility case, mpsPrefixFields is redundant since you can discard entity when specifying the mpsFieldLabelModifier.

I propose that we deprecate mpsPrefixFields and document that mpsFieldLabelModifier will be ignored if mpsPrefixFields is set to False.
This way, we avoid complicating the type of the fieldLabelModifier.

WDYT?

[parsonsmatt]

That sounds good to me!

[olivierdeckers]

Observation 2 is not completely correct. mpsPrefixFields is also used for disabling the entity prefix in the constraint name, which should be uppercase and not start with an underscore.
One solution is to reuse the modifier function, drop any leading underscore and uppercase the result, but maybe an additional modifier function for the constraints would make more sense?

[olivierdeckers]

I went ahead and implemented a second modifier function (mpsConstraintLabelModifier) for the constraints which need to be upper case. Let me know what you think

[parsonsmatt]

Looks great to me! Please commit the suggestions for the version numbers and we'll be set to roll.

[parsonsmatt]

We can use a DEPRECATED pragma to make this official.

[parsonsmatt]

Suggested change

	## 2.10.5.3
	## 2.11.0.0

[parsonsmatt]

Suggested change

	version: 2.10.5.3
	version: 2.11.0.0