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

[Documentation] Replaced type with [type] #6738

Open
wants to merge 5 commits into
base: feature/efm-recovery
Choose a base branch
from

Conversation

durkmurder
Copy link
Member

Context

Based on comment on recent PR this PR replaces usages of `type` with [type].

I have used following regex to find all type usages: `([a-zA-Z]+.[a-zA-Z]+)` and then next expression to replace: [$1]

@durkmurder durkmurder changed the title [Documentation] Replaced \type\ with [type] [Documentation] Replaced \ type \ with [type] Nov 19, 2024
@durkmurder durkmurder changed the title [Documentation] Replaced \ type \ with [type] [Documentation] Replaced type with [type] Nov 19, 2024
@durkmurder durkmurder changed the title [Documentation] Replaced type with [type] [Documentation] Replaced type with [type] Nov 19, 2024
@durkmurder durkmurder changed the title [Documentation] Replaced type with [type] [Documentation] Replaced \type\ with [type] Nov 19, 2024
@durkmurder durkmurder changed the title [Documentation] Replaced \type\ with [type] [Documentation] Replaced type with [type] Nov 19, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.

Project coverage is 41.72%. Comparing base (d0c9695) to head (f7fbe57).

Files with missing lines Patch % Lines
consensus/hotstuff/validator/validator.go 57.14% 3 Missing ⚠️
engine/consensus/dkg/reactor_engine.go 0.00% 1 Missing ⚠️
module/dkg/client.go 0.00% 1 Missing ⚠️
network/validator/pubsub/topic_validator.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                    @@
##           feature/efm-recovery    #6738      +/-   ##
========================================================
+ Coverage                 41.70%   41.72%   +0.01%     
========================================================
  Files                      2030     2030              
  Lines                    180462   180462              
========================================================
+ Hits                      75261    75295      +34     
+ Misses                    99003    98976      -27     
+ Partials                   6198     6191       -7     
Flag Coverage Δ
unittests 41.72% <84.21%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Thanks for updating the documentation to add links. Unfortunately, the regex approach is not very accurate (I added a few comments pointing out places where the result won't work as expected).

Honestly I'm not sure it is worth our time to go back to fix existing type references (filtering through the diff to find the places where the regex didn't work). Rather, I think we should just keep this in mind for future code changes.

@@ -2733,7 +2733,7 @@ func TestTypeRequirementRemovalMigration(t *testing.T) {

contractName := "TiblesProducer"

// Store a value with the actual `TiblesProducer.Minter` type
// Store a value with the actual [TiblesProducer.Minter] type
Copy link
Member

Choose a reason for hiding this comment

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

This is referencing a Cadence smart contract, so the [] won't resolve as a link.

@@ -82,7 +82,7 @@ var _ ProposalTiming = (*happyPathBlockTime)(nil)
// In other words, when primary determines at what future time to broadcast the child, the child
// has _not_ been published and the `timedBlock` references the parent on the happy path (or another
// earlier block on the unhappy path)
// - `unconstrainedBlockTime` is the delay, relative to `timedBlock.TimeObserved` when the controller would
// - `unconstrainedBlockTime` is the delay, relative to [timedBlock.TimeObserved] when the controller would
Copy link
Member

Choose a reason for hiding this comment

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

timedBlock is an unexported name, so this won't resolve as a link.

// timeout for this view. When the timer for the view's original duration expires, a `TimeoutObject`
// with `TimeoutTick = 0` is broadcast. Subsequently, `timeout.Controller` re-broadcasts the
// with `TimeoutTick = 0` is broadcast. Subsequently, [timeout.Controller] re-broadcasts the
Copy link
Member

Choose a reason for hiding this comment

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

This won't resolve as a link because it is documenting a struct field.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Good idea to link the Go types in docstrings! However, there seem to be changes outside of docstrings that should be reverted

@durkmurder
Copy link
Member Author

@jordanschalm @turbolent It took me 5 mins to build a regex and replace everything. Pretty much low effort if you ask me. Even if it's not 100% accurate it shouldn't harm replacing all `` with []. As long as tests are passing I am pretty confident that it shouldn't cause a regression.

@turbolent
Copy link
Member

Oh yeah, I think replacing linking all occurrences in docstrings if fine, even if they might not work, as long as we don't change non-docstring code 👍

@@ -250,7 +250,7 @@ func TestBlockContext_DeployContract(t *testing.T) {
require.NoError(t, output.Err)
})

t.Run("account with deployed contract has `contracts.names` filled", func(t *testing.T) {
t.Run("account with deployed contract has [contracts.names] filled", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Lets revert this, because its part of an code-string

@@ -211,7 +211,7 @@ func (suite *EpochLookupSuite) TestProtocolEvents_EpochExtended_SanityChecks() {
return ctx
}

suite.T().Run("sanity check: `extension.FinalView` should be greater than final view of latest epoch", func(t *testing.T) {
suite.T().Run("sanity check: [extension.FinalView] should be greater than final view of latest epoch", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

in-code string, which I would suggest to not modify

@@ -144,7 +144,7 @@ func (suite *Suite) TestCancelVoting() {
go func() {
err := suite.voter.Vote(ctxWithCancel, suite.epoch)
suite.Assert().Error(err, "when canceling voting process, Vote method should return with an error")
suite.Assert().ErrorIs(err, context.Canceled, "`context.Canceled` should be in the error trace")
suite.Assert().ErrorIs(err, context.Canceled, "[context.Canceled] should be in the error trace")
Copy link
Member

Choose a reason for hiding this comment

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

in-code string, which I would suggest to not modify

// the error should be escalated to the caller.
t.Run("low-level `ProtocolKVStore.ByBlockID` errors", func(t *testing.T) {
t.Run("low-level [ProtocolKVStore.ByBlockID] errors", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

in-code string, which I would suggest to not modify

// the error should be escalated to the caller.
t.Run("low-level `ProtocolKVStore.ByID` errors", func(t *testing.T) {
t.Run("low-level [ProtocolKVStore.ByID] errors", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

in-code string, which I would suggest to not modify

@@ -633,7 +633,7 @@ func (m *mockStateTransition) Mock() protocol_statemock.OrthogonalStoreStateMach
deferredUpdate.On("Execute", mock.Anything).Return(nil).Once()
deferredDBUpdates := transaction.NewDeferredBlockPersist().AddDbOp(deferredUpdate.Execute)
stateMachine.On("Build").Run(func(args mock.Arguments) {
require.True(m.T, evolveStateCalled, "Method `OrthogonalStoreStateMachine.Build` called before `EvolveState`!")
require.True(m.T, evolveStateCalled, "Method [OrthogonalStoreStateMachine.Build] called before `EvolveState`!")
Copy link
Member

Choose a reason for hiding this comment

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

in-code string, which I would suggest to not modify

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.

5 participants