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

Case insensitive chain name #533

Merged
merged 6 commits into from
Jul 6, 2021
Merged

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented Jul 5, 2021

Description

This PR makes the name of ChainConfig case insensitive.
Closes #526

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Wrote integration tests (simulation & CLI).
  • Updated the documentation.
  • Added an entry to the CHANGELOG.md file.
  • Re-reviewed Files changed in the Github PR explorer.

@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #533 (0b78076) into master (93f8514) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 0b78076 differs from pull request most recent head 37fbc2f. Consider uploading reports for the commit 37fbc2f to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #533   +/-   ##
=======================================
  Coverage   81.81%   81.82%           
=======================================
  Files          91       91           
  Lines        4734     4736    +2     
=======================================
+ Hits         3873     3875    +2     
  Misses        667      667           
  Partials      194      194           
Impacted Files Coverage Δ
x/profiles/types/models_chain_links.go 87.00% <100.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93f8514...37fbc2f. Read the comment docs.

@dadamu dadamu force-pushed the paul/chain-name-case-insensitive branch from 2a58b51 to 04f72b6 Compare July 5, 2021 07:34
@dadamu dadamu marked this pull request as ready for review July 6, 2021 04:50
@dadamu dadamu requested review from leobragaz and RiccardoM July 6, 2021 04:50
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

The implementation looks good, but I'm having a hard time deciding whether this is worth or it might have some backslash in the future

@@ -22,7 +22,7 @@ import (
// NewChainConfig allows to build a new ChainConfig instance
func NewChainConfig(name string) ChainConfig {
return ChainConfig{
Name: name,
Name: strings.ToLower(name),
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 this should be removed if we want the Validate method to fail

@leobragaz
Copy link
Contributor

@RiccardoM why backslash? I don't think we will see chains where name differs only for some case letter.

CHANGELOG.md Outdated Show resolved Hide resolved
@RiccardoM RiccardoM merged commit 66e0c98 into master Jul 6, 2021
@RiccardoM RiccardoM deleted the paul/chain-name-case-insensitive branch July 6, 2021 09:42
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.

Make the name of ChainConfig case insensitive
3 participants