Skip to content

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Mar 10, 2020

Drop unused functions:

  • ParseQueryParamBool()
  • ParseInt64OrReturnBadRequest()

For contributor use:

  • 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.
  • Code follows the module structure standards.
  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Drop unused functions:
- ParseQueryParamBool()
- ParseInt64OrReturnBadRequest()
@alessio alessio force-pushed the alessio/rest-tests branch from a608546 to 5d1156a Compare March 10, 2020 16:50
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Weird -- I swear these were used before. ACK

@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 10, 2020
@alessio alessio removed the A:automerge Automatically merge PR once all prerequisites pass. label Mar 10, 2020
@alessio
Copy link
Contributor Author

alessio commented Mar 10, 2020

@alexanderbez dixit:

Weird -- I swear these were used before. ACK

I was surprised too! AFAICS ParseInt64OrReturnBadRequest was introduced first in #1918, and seems to have been deprecated in favor of ParseUint64OrReturnBadRequest over time. Whilst ParseQueryParamBool was introduced by @fedekunze in #5585. @fedekunze: should we keep that function?

While waiting for a response from @fedekunze, I've removed the automerge label. If that function is here to stay, I'd like to see a test case for it - I can write one this time, no probs.

@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #5779 into master will increase coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5779      +/-   ##
==========================================
+ Coverage   35.91%   36.07%   +0.16%     
==========================================
  Files         341      341              
  Lines       34745    34731      -14     
==========================================
+ Hits        12478    12530      +52     
+ Misses      21023    20955      -68     
- Partials     1244     1246       +2
Impacted Files Coverage Δ
types/rest/rest.go 86.33% <ø> (+32.53%) ⬆️

@alessio alessio added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 10, 2020
@alessio
Copy link
Contributor Author

alessio commented Mar 10, 2020

Nevermind, I've reintroduced the function for now (and written a test case)

@mergify mergify bot merged commit 2de4775 into master Mar 10, 2020
@mergify mergify bot deleted the alessio/rest-tests branch March 10, 2020 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants