Add ListBGPSpeakers (continued)#2229
Conversation
1. Change BGPSpeakerPage to SinglePageBase 2. Change the return type of ListBGPSpeakers to ListBGPSpeakersResult 3. Implement the Extract method of ListBGPSpeakersResult in agents 4. misc: delete unused stuff for now and change the wording of doc.go
|
Build failed.
|
| } | ||
|
|
||
| // Extract inteprets the ListBGPSpeakersResult into an array of BGP speakers | ||
| func (r ListBGPSpeakersResult) Extract() ([]speaker.BGPSpeaker, error) { |
There was a problem hiding this comment.
The naming pattern/convention for methods that return slices is ExtractResourceName. So in this case, this method should be ExtractBGPSpeakers. Please ignore the DHCP implementation in this package - there are a lot of errors with it.
|
|
||
| // ListBGPSpeakersResult is the respone of agents/{id}/bgp-speakers | ||
| type ListBGPSpeakersResult struct { | ||
| gophercloud.Result |
There was a problem hiding this comment.
This should be a SinglePageBase
| @@ -0,0 +1,69 @@ | |||
| package speaker | |||
There was a problem hiding this comment.
The package name should be plural: speakers.
|
Hi @jtopjian Thanks for the review. I've updated the code per your request. Can you have a look at it? Thanks |
|
Build failed.
|
|
|
||
| Example to List BGP speakers by dragent | ||
|
|
||
| pages, err := agents.ListDRAgentHostingBGPSpeakers(client, agentID).AllPages() |
There was a problem hiding this comment.
The original method name for this was correct: ListBGPSpeakers.
To clarify: in a previous PR I suggested the name of something like ListByHostingAgent. This was in the context of if the code was hosted under the speakers package. If you are choosing to continue to host this code under the agents package, then the original ListBGPSpeakers name is correct.
There was a problem hiding this comment.
@jtopjian
Sorry, my bad. I've fixed it. Please have a look.
|
Build failed.
|
jtopjian
left a comment
There was a problem hiding this comment.
@shhgs I apologize, it looks like there are some formatting errors that the unit tests are reporting:
https://github.com/gophercloud/gophercloud/pull/2229/checks?check_run_id=3872039117
GitHub has implemented a policy where Actions are not automatically run for first-time contributors, which is why this wasn't reported earlier.
Just run go fmt on all of the modified files and that should fix everything. Additionally, go vet might pick up some items, too.
|
There was an empty line in fixtures.go. I am using vim-go, which is suppose to call gofmt everytime it writes the file. Anyway it is fixed. |
|
Build failed.
|
jtopjian
left a comment
There was a problem hiding this comment.
LGTM for real this time.
Hi @jtopjian,
I pull rebase this branch against the origin/master, and then run
push -fwithout knowing that will close the original pull request. So I create this one to replace old one.Thanks
-Jon
For #2208