Skip to content

Rename converter transformer#77

Merged
m0ar merged 2 commits intostagingfrom
rename-converter-transformer
Jan 7, 2020
Merged

Rename converter transformer#77
m0ar merged 2 commits intostagingfrom
rename-converter-transformer

Conversation

@m0ar
Copy link
Copy Markdown
Contributor

@m0ar m0ar commented Dec 18, 2019

💯

@m0ar m0ar self-assigned this Dec 18, 2019
@m0ar
Copy link
Copy Markdown
Contributor Author

m0ar commented Dec 18, 2019

Like I said in the call I'm unable to debug this locally since I've got DB troubles, but shouldn't be anything big.

@rmulhol I remember touching some tests where additional parameters were required for maxRetry etc on the new watcher functionality. IDK why this was even missing, maybe I screwed something up? 🤔

@gslaughl gslaughl force-pushed the rename-converter-transformer branch from 9a09eab to 7a51a29 Compare January 2, 2020 05:44
Copy link
Copy Markdown
Contributor

@elizabethengelman elizabethengelman left a comment

Choose a reason for hiding this comment

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

👽LGTM!

constants.AddressColumn: rawDealLog.Address.Hex(),
event.LogFK: DealHeaderSyncLog.ID,
event.HeaderFK: DealHeaderSyncLog.HeaderID,
event.AddressFK: rawDealLog.Address.Hex(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that most of the tests that use these models for assertions, we look up the address id in the test itself rather than relying on this hex string. So maybe we should remove this from the test model since it's not an accurate representation of what a model will look like similar to how we're handling it for test dent?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice catch! Yeah that hex definitely doesn't belong there.

Remove constants.AddressColumn in favour of event.AddressFK

Fix formatting + tests
@gslaughl gslaughl force-pushed the rename-converter-transformer branch from 7a51a29 to d8c768f Compare January 3, 2020 16:49
@m0ar m0ar merged commit f5229ed into staging Jan 7, 2020
@m0ar m0ar deleted the rename-converter-transformer branch January 7, 2020 13:33
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.

3 participants