Skip to content

Fixes name shadowing error at keyFromRecordM for Primary keys#1496

Merged
parsonsmatt merged 4 commits intoyesodweb:masterfrom
hw202207:haisheng/shadow-binding-on-primary
Jun 27, 2023
Merged

Fixes name shadowing error at keyFromRecordM for Primary keys#1496
parsonsmatt merged 4 commits intoyesodweb:masterfrom
hw202207:haisheng/shadow-binding-on-primary

Conversation

@hw202207
Copy link
Contributor

@hw202207 hw202207 commented May 18, 2023

With a persistent definition like,

Customer
  firstName String
  lastName String
  Primary firstName lastName

the keyFromRecordM was generated like

keyFromRecordM = Just
            (\ Customer {customerFirstName = customerFirstName,
                         customerLastName = customerLastName}
               -> (CustomerKey customerFirstName) customerLastName)

hence the name-shadowing error.

The fixes is simply append index to those variables, like

keyFromRecordM
        = Just
            (\ Customer {customerFirstName = customerFirstName_1,
                         customerLastName = customerLastName_2}
               -> (CustomerKey customerFirstName_1) customerLastName_2)

Before submitting your PR, check that you've:

  • Ran stylish-haskell on any changed files.

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)

hw202207
hw202207 commented May 18, 2023
View reviewed changes
persistent/Database/Persist/TH.hs
conp :: Name -> [Pat] -> Pat
conp name pats = ConP name [] pats
#else
conp :: Name -> [Pat] -> Pat
Copy link
Contributor Author

@hw202207 hw202207 May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes an annoying warning.
Unrelated but simple it's simple enough so I just keep it.
I could revert if you prefer.

hw202207
hw202207 commented May 18, 2023
View reviewed changes
persistent/test/Database/Persist/TH/CompositeKeyStyleSpec.hs
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE UndecidableInstances #-}

{-# OPTIONS_GHC -Wname-shadowing -Werror=name-shadowing #-}
Copy link
Contributor Author

@hw202207 hw202207 May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To verify the fixes actually work.

@hw202207
Copy link
Contributor Author

hw202207 commented May 18, 2023

Just found sort of duplicated with #1494
which looks better (used newName instead of mkName)

This was referenced May 30, 2023
Closed
Closed
@parsonsmatt
Copy link
Collaborator

parsonsmatt commented Jun 1, 2023

Yeah, let's go with the newName approach - I like the testing/verification changes you made here though. Do you mind folding that PR into this one?

@hw202207
Copy link
Contributor Author

hw202207 commented Jun 1, 2023

With the newName, the result looks like this

keyFromRecordM
        = Just
            (\ CompanyUserCamelStyle {companyUserCamelStyleCompanyName = companyUserCamelStyleCompanyName_acxmR,
                                      companyUserCamelStyleUserName = companyUserCamelStyleUserName_acxmS}
               -> (CompanyUserCamelStyleKey
                     companyUserCamelStyleCompanyName_acxmR)
                    companyUserCamelStyleUserName_acxmS)

given definition

mkPersist sqlSettings {mpsCamelCaseCompositeKeySelector = True}
  [persistLowerCase|
    CompanyUserCamelStyle
      companyName Text
      userName Text
      Primary companyName userName
  |]

parsonsmatt
parsonsmatt approved these changes Jun 1, 2023
View reviewed changes
Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

@parsonsmatt parsonsmatt merged commit 2a26141 into yesodweb:master Jun 27, 2023
@parsonsmatt parsonsmatt mentioned this pull request Jun 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@parsonsmatt parsonsmatt parsonsmatt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hw202207 @parsonsmatt