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

Feature/named types #245

Merged
merged 13 commits into from Apr 14, 2021
Merged

Feature/named types #245

merged 13 commits into from Apr 14, 2021

Conversation

arnabmitra
Copy link
Contributor

@arnabmitra arnabmitra commented Apr 12, 2021

Description

Typed Events and Metric counters in Name Module #85

closes: #85


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #245 (009373c) into main (995c8f6) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #245      +/-   ##
==========================================
- Coverage   49.39%   49.39%   -0.01%     
==========================================
  Files         115      117       +2     
  Lines       10477    10544      +67     
==========================================
+ Hits         5175     5208      +33     
- Misses       4783     4816      +33     
- Partials      519      520       +1     
Impacted Files Coverage Δ
x/name/keeper/msg_server.go 1.28% <0.00%> (-0.15%) ⬇️
x/name/handler.go 64.70% <0.00%> (ø)
x/name/module.go 52.38% <0.00%> (ø)

@arnabmitra
Copy link
Contributor Author

this does break backwards compatibility for event names

/ before name of the event was `types.EventTypeNameBound` == name_bound
	// because proto message format's do not encourage _ like convention
	// but prefer CamelCase for message name, NameBound
	// https://developers.google.com/protocol-buffers/docs/style
	// Use CamelCase (with an initial capital) for message names – for example, SongServerRequest.
	// Use underscore_separated_names for field names (including oneof field and extension names) – for example, song_name.
	// Emit event and return
	// Sample event:
	// [{"events":[{"type":"message","attributes":[{"key":"action","value":"bind_name"},{"key":"sender","value":"tp13ulywwfe7v38y0vetsqayccsgzexh6zq38h3d4"}]},{"type":"provenance.name.v1.EventNameBound","attributes":[{"key":"address","value":"\"tp13ulywwfe7v38y0vetsqayccsgzexh6zq38h3d4\""},{"key":"name","value":"\"sc1.pb\""}]},{"type":"transfer","attributes":[{"key":"recipient","value":"tp17xpfvakm2amg962yls6f84z3kell8c5l2udfyt"},{"key":"sender","value":"tp13ulywwfe7v38y0vetsqayccsgzexh6zq38h3d4"},{"key":"amount","value":"2000nhash"}]}]}]

@arnabmitra
Copy link
Contributor Author

Did add handler_test.go which test's out emission of events.

@arnabmitra arnabmitra marked this pull request as ready for review April 14, 2021 15:17
Copy link
Contributor

@channa-figure channa-figure left a comment

Choose a reason for hiding this comment

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

LGTM. I will follow the same test structure in marker module for confirming the event.

Copy link
Contributor

@channa-figure channa-figure left a comment

Choose a reason for hiding this comment

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

Sorry, for approving then requesting changes. I notice when I checked out your branch and ran the go formatter it made some changes. Are you running that after saves?

@arnabmitra
Copy link
Contributor Author

arnabmitra commented Apr 14, 2021

Sorry, for approving then requesting changes. I notice when I checked out your branch and ran the go formatter it made some changes. Are you running that after saves?

good question, when i started i noticed golangci-lint run --fix would fix gofmt error's on the linter, and assumed it runs gofmt if not run..(ya checked it runs gofmt on all files, version is golangci-lint --version
golangci-lint has version 1.38.0 built from 507703b on 2021-03-03T14:50:52Z
)
what do you guys run? i can follow the same convention

…fmt we are using overall or wht golangcli-lint does not format these.
@channa-figure
Copy link
Contributor

@arnabmitra I know we talked about this in private, but I will post it publicly here.

I don't run the gofmt command-line through all the files right now (it would muddy up the PR too much IMO). My editors emacs and vscode do it for me on save for the files I'm editing. Maybe we should have a PR to format all the files and then add some enforcement to PR's?

Copy link
Contributor

@channa-figure channa-figure left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@arnabmitra
Copy link
Contributor Author

@arnabmitra I know we talked about this in private, but I will post it publicly here.

I don't run the gofmt command-line through all the files right now (it would muddy up the PR too much IMO). My editors emacs and vscode do it for me on save for the files I'm editing. Maybe we should have a PR to format all the files and then add some enforcement to PR's?

i think the linter check does check the fmt.. for e.g if you check in a badly formatted file ..like add several tabs..it will complain..when i did my first PR i had a bunch of gofmt errors in the linter..

@arnabmitra arnabmitra merged commit 62720b5 into main Apr 14, 2021
@arnabmitra arnabmitra deleted the feature/named-types branch April 14, 2021 19: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.

Typed Events and Metric counters in Name Module
2 participants