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

Rename Tag(s) to Event(s) #4046

Merged
merged 19 commits into from Dec 4, 2019
Merged

Rename Tag(s) to Event(s) #4046

merged 19 commits into from Dec 4, 2019

Conversation

tac0turtle
Copy link
Contributor

  • tag was replaced with event, but in some places it still mentions tag, would be easier to understand if we tried to replace it everywhere with event.

Signed-off-by: Marko Baricevic marbar3778@yahoo.com

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

- tag was replaced with event, but in some places it still mentions tag, would be easier to understand if we tried to replace it with event to not confuse people.

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
@tac0turtle tac0turtle added T:breaking Type: Breaking Change T:code-hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. R:major PR contains breaking changes that have to wait till a major release is made to be merged labels Oct 8, 2019
@tac0turtle tac0turtle self-assigned this Oct 8, 2019
config/config.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 11, 2019

Codecov Report

Merging #4046 into master will decrease coverage by 0.01%.
The diff coverage is 68.18%.

@@            Coverage Diff             @@
##           master    #4046      +/-   ##
==========================================
- Coverage   67.27%   67.25%   -0.02%     
==========================================
  Files         223      223              
  Lines       19342    19342              
==========================================
- Hits        13012    13009       -3     
- Misses       5349     5355       +6     
+ Partials      981      978       -3
Impacted Files Coverage Δ
libs/pubsub/query/empty.go 50% <ø> (ø) ⬆️
libs/pubsub/query/query.go 63.86% <ø> (ø) ⬆️
libs/pubsub/subscription.go 91.3% <ø> (ø) ⬆️
state/txindex/indexer_service.go 66.66% <ø> (ø) ⬆️
config/toml.go 65.95% <ø> (ø) ⬆️
node/node.go 62.95% <0%> (ø) ⬆️
config/config.go 83.97% <100%> (ø) ⬆️
state/txindex/kv/kv.go 74.52% <81.25%> (ø) ⬆️
blockchain/v2/reactor.go 50.87% <0%> (-8.78%) ⬇️
blockchain/v2/routine.go 81.81% <0%> (-6.07%) ⬇️
... and 6 more

@melekes
Copy link
Contributor

melekes commented Nov 11, 2019

#4077 (comment)

We need to agree on naming here. In #4077 I've proposed the new name - compositeKey (because it's composite of event.Type and event.AttributeName) for what we were previously callingtag. Let me know if you have a better idea!

@tac0turtle
Copy link
Contributor Author

I am fine with that, makes more sense, will make the adjustments,

@tac0turtle tac0turtle marked this pull request as ready for review November 15, 2019 13:41
config/config.go Outdated
@@ -875,7 +875,7 @@ func (cfg *ConsensusConfig) ValidateBasic() error {
// TxIndexConfig

// TxIndexConfig defines the configuration for the transaction indexer,
// including tags to index.
// including compositeKeys to index.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll need to explain what compositeKey is here:

Remember that Event have a following structure:
type: [
  key: value,
  ...
]

composite key is a type and key, separated by a single dot. For example, given the Event below

"jack": [
  "account.number": 100
]

composite key will be jack.account.number

config/config.go Outdated Show resolved Hide resolved
docs/app-dev/indexing-transactions.md Outdated Show resolved Hide resolved
docs/app-dev/indexing-transactions.md Outdated Show resolved Hide resolved
docs/app-dev/indexing-transactions.md Outdated Show resolved Hide resolved
state/txindex/kv/kv.go Outdated Show resolved Hide resolved
state/txindex/kv/kv.go Outdated Show resolved Hide resolved
types/event_bus.go Outdated Show resolved Hide resolved
types/events.go Outdated Show resolved Hide resolved
types/event_bus_test.go Outdated Show resolved Hide resolved
Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com>
config/toml.go Show resolved Hide resolved
docs/app-dev/indexing-transactions.md Outdated Show resolved Hide resolved
docs/tendermint-core/configuration.md Show resolved Hide resolved
state/txindex/kv/kv_bench_test.go Outdated Show resolved Hide resolved
state/txindex/kv/kv_bench_test.go Outdated Show resolved Hide resolved
config/toml.go Show resolved Hide resolved
@melekes
Copy link
Contributor

melekes commented Dec 3, 2019

we will need a changelog entry since we're breaking the config file (old config files won't work with new TM)

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👭 🚀 🌔

@@ -70,14 +72,14 @@ program](https://hackerone.com/tendermint).
- Blockchain Protocol

- [abci] \#2521 Remove `TotalTxs` and `NumTxs` from `Header`
- [types] [\#4151](https://github.com/tendermint/tendermint/pull/4151) Enforce ordering of votes in DuplicateVoteEvidence to be lexicographically sorted on BlockID
- [types][\#4151](https://github.com/tendermint/tendermint/pull/4151) Enforce ordering of votes in DuplicateVoteEvidence to be lexicographically sorted on BlockID
Copy link
Contributor

Choose a reason for hiding this comment

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

🤣 so I am adding white spaces and you're deleting them, nice

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
@tac0turtle tac0turtle merged commit 92d18d7 into master Dec 4, 2019
@tac0turtle tac0turtle deleted the marko/rename-Tag-Event branch December 4, 2019 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R:major PR contains breaking changes that have to wait till a major release is made to be merged T:breaking Type: Breaking Change T:code-hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants