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.

Sign up for GitHub

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

Jump to bottom

Get rid of Request.convertOptions #183

Closed
Stebalien opened this issue Mar 12, 2020 · 5 comments
Closed

Get rid of Request.convertOptions #183

Stebalien opened this issue Mar 12, 2020 · 5 comments
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue

Comments

@Stebalien
Copy link
Member

Currently, we:

  1. Extract the options from the request parameters.
  2. Stash them in the request options map.
  3. Later, convert the options to the correct types, storing them back in the request.

This is bad practice. We should:

  1. Extract the options from the request parameters.
  2. Convert them.
  3. Save them.

That is, we should never store temporary data in the place we expect to store the final data, then fix it up later.
@hsanjuan hsanjuan added exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue labels Mar 13, 2020
@gargdeepak
Copy link

Can I please pick up this issue if no one else is working on it ?

@Stebalien
Copy link
Member Author

That would be awesome! Fair warning, the code in this repo has been patched heavily and is a bit gnarly.

gargdeepak pushed a commit to gargdeepak/go-ipfs-cmds that referenced this issue Mar 26, 2020
ipfs#183 refactored the request options conversion code
9084337 
Now options are converted and saved before the request object is generated
@gargdeepak
Copy link

Thanks @Stebalien !
I have pushed the fix here: https://github.com/gargdeepak/go-ipfs-cmds/tree/fix/cmds/req-refactor
I ran go tests and following the contribution guidelines.
Anything else I should do before sending out a PR ?

@Stebalien
Copy link
Member Author

Go ahead and file a PR!

@gargdeepak
Copy link

needs review

gargdeepak pushed a commit to gargdeepak/go-ipfs-cmds that referenced this issue Mar 27, 2020
Fixes ipfs#183. Refactored the method signature as commented on the PR
b432553
gargdeepak pushed a commit to gargdeepak/go-ipfs-cmds that referenced this issue Mar 27, 2020
Fixes ipfs#183. Per PR comments, not modifying the mutating opts object
1b028fb
gargdeepak pushed a commit to gargdeepak/go-ipfs-cmds that referenced this issue Mar 27, 2020
Fixes ipfs#183. Not modifying the mutating opts object per comments
c9accce
Stebalien added a commit that referenced this issue Mar 30, 2020
@Stebalien
Merge pull request #187 from gargdeepak/fix/cmds/req-refactor
3de51c6 
#183 refactored the request options conversion code per the ticket requirements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Stebalien @gargdeepak @hsanjuan