Skip to content

feat(dot/types): moves ParachainInherentData type to types package#3048

Closed
EclesioMeloJunior wants to merge 1 commit intodevelopmentfrom
eclesio/move-parachain-primitives
Closed

feat(dot/types): moves ParachainInherentData type to types package#3048
EclesioMeloJunior wants to merge 1 commit intodevelopmentfrom
eclesio/move-parachain-primitives

Conversation

@EclesioMeloJunior
Copy link
Copy Markdown
Member

Changes

  • Extract ParachainInherentData type from lib/babe package and moves it to dot/types package, this is needed since there is a bunch of tests that need it but cannot import due to cyclic dependency.

Tests

N/A

Issues

Primary Reviewer

@timwu20
@qdm12

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 16, 2023

Codecov Report

Merging #3048 (d7c786f) into development (6255f39) will increase coverage by 0.04%.
The diff coverage is 0.00%.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #3048      +/-   ##
===============================================
+ Coverage        51.43%   51.47%   +0.04%     
===============================================
  Files              219      219              
  Lines            28009    28009              
===============================================
+ Hits             14406    14419      +13     
+ Misses           12318    12306      -12     
+ Partials          1285     1284       -1     

Copy link
Copy Markdown
Contributor

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

👍 Although I would instead have it in the interrnal/ directory to limit the external usage of these types from outside repositories.

Crossing my mind are:

  • internal/babe/parachain
  • internal/babe/inherents
  • internal/primitives (=internal replacement for dot/types)
  • internal/primitives/parachain
  • internal/primitives/inherents

If not, maybe it would be the time to separate things in the dot/types package and add a dot/types/parachain package, to reduce the tight coupling/dependency contention.

PS: Forgive my opinionated opinion 😄

@kishansagathiya
Copy link
Copy Markdown
Contributor

I like lib/babe/types or lib/babe/inherents if that solves your cyclical dependencies.

Copy link
Copy Markdown
Contributor

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

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

you could have just added this change as part of #3047

That way reviewer would have more context of circular dependency

@qdm12
Copy link
Copy Markdown
Contributor

qdm12 commented Jan 17, 2023

you could have just added this change as part of #3047

That way reviewer would have more context of circular dependency

Good point, @EclesioMeloJunior does #3047 modify these files? If not we can move those changes to #3047 and close this PR? File movements will be tracked correctly even if commits are squashed, as long as we don't modify these babe files too much.

I like lib/babe/types or lib/babe/inherents if that solves your cyclical dependencies.

  • In ./internal/ would be better in order to hide from public before our Go API is stable / needs to be used outside
  • I like lib/babe/inherents
  • We should stop throwing moareee in the the kitchen sink aka dot/types

@EclesioMeloJunior
Copy link
Copy Markdown
Member Author

I will include these changes in #3047, and I will move these files to lib/babe/inherents

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