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

add option to list to show column with state #102

Merged
merged 6 commits into from
Feb 24, 2022
Merged

add option to list to show column with state #102

merged 6 commits into from
Feb 24, 2022

Conversation

operte
Copy link
Collaborator

@operte operte commented Feb 22, 2022

Solves one of the open items from #94

@ClaudioSalvatoreArcidiacono
Copy link
Contributor

I am not super familiar with the package so I am not sure about that, but if you have some documentation about the list command you should also add this new option to it.

@ClaudioSalvatoreArcidiacono
Copy link
Contributor

ClaudioSalvatoreArcidiacono commented Feb 23, 2022

I see that there is no change in tests. Because there are no tests on this part. Would it be the case to add a couple of tests for the list function?

@operte
Copy link
Collaborator Author

operte commented Feb 23, 2022

I see that there is no change in tests. Because there are no tests on this part. Would it be the case to add a couple of tests for the list function?

@ClaudioSalvatoreArcidiacono That's one of the main things missing in this project: tests. We still didn't try to find a good way to test these functions. I guess this will have to be done with mock classes that could mimic the replies from the azure devops APIs. We could also try something even simpler, to begin with, and just check that we are sending out the correct API requests. Not really sure what's the best way to go about it.

@ClaudioSalvatoreArcidiacono
Copy link
Contributor

We could also try something even simpler, to begin with, and just check that we are sending out the correct API requests. Not really sure what's the best way to go about it.

I usually use httmock for such kind of things. It is great :)
https://github.com/patrys/httmock

@ClaudioSalvatoreArcidiacono
Copy link
Contributor

We could also try something even simpler, to begin with, and just check that we are sending out the correct API requests. Not really sure what's the best way to go about it.

I usually use httmock for such kind of things. It is great :) https://github.com/patrys/httmock

But for the functions you modified, you do not really need to mock the http layer. You could just check the Table object that is returned by build_table for example

@operte
Copy link
Collaborator Author

operte commented Feb 23, 2022

I added a test, but just for the new functionality added by this PR.

I will fix the import sorting in a different PR, since there are more files that need reorganizing. I'll also add isort to the pre commit hooks :) And probably remove mypy :P

operte and others added 2 commits February 24, 2022 09:46
@operte
Copy link
Collaborator Author

operte commented Feb 24, 2022

Thanks for the comments! Much appreciated!

@operte operte merged commit 1d51fd5 into main Feb 24, 2022
@operte operte deleted the list-show-state branch February 24, 2022 09:30
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.

2 participants