Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring name generating functions in TH #1218

Merged
merged 29 commits into from
Apr 20, 2021

Conversation

danbroooks
Copy link
Contributor

@danbroooks danbroooks commented Mar 29, 2021

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Bumped the version number if there isn't an (unreleased) on the Changelog
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

Refactors the TH module, specifically themed around generating TH Names, or Exp values that use those names.

Now I think this module is a lot dry'er, where we have some duplication currently scattered around this module dealing with how names are generated.

For example, the Entity name when using mpsGeneric was duplicated in 3 different places, this change has DRY'd this up into a single function.

I considered moving these functions out and into their own module, and adding some HSpec tests for each name generating function, for the key name generating functions at least, such as the constructor name/field accessor names, but thought this might be over kill. I think this functionality is largely covered in persistent-test so thought best of doing that. I have grouped these functions at the bottom of the module anyway.

I think there is probably more we can do here to make this module a bit easier to follow in line with #1156 while also trying to balance out not changing too much in a single change request. Hopefully there are some values/functions in here now with some slightly clearer names too.

I took a very incremental approach to this change, so there are a lot of commits here. I've left them as they are for now, but if it is preferred that they're squashed into a single commit I can do so 👍

@parsonsmatt
Copy link
Collaborator

this is great, thanks! 😄

i'm going to make a 2.12.1.1 patch release soon, do you want to add a note in the changelog for this?

@parsonsmatt parsonsmatt merged commit 1a83621 into yesodweb:master Apr 20, 2021
@danbroooks danbroooks mentioned this pull request Apr 22, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants