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

fix CLI logic and update tests #125

Merged
merged 4 commits into from
Oct 18, 2021
Merged

fix CLI logic and update tests #125

merged 4 commits into from
Oct 18, 2021

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Oct 9, 2021

As a follow-up to #123 this PR tries to fix the logic of the CLI and to update the test cases after the API in the light of the commander changes. There are currently still some issues present which is why I opened this PR as a draft for now. Feel free to already leave some review comments if you like :)

Once this PR is being merged, it will fix #122.

Edit: It now also fixes #56.

@JKRhb JKRhb marked this pull request as draft October 10, 2021 09:48
@gerdemann
Copy link
Member

gerdemann commented Oct 12, 2021

Thanks for your PR!

@gerdemann gerdemann marked this pull request as ready for review October 13, 2021 06:00
@gerdemann gerdemann closed this Oct 13, 2021
@gerdemann gerdemann reopened this Oct 13, 2021
@gerdemann
Copy link
Member

Can you tell me what other issues exist and maybe fix them?
I have also set up Github Actions, it seems to be working.

@gerdemann
Copy link
Member

This PR looks good so far, it can be merged, right?

@JKRhb
Copy link
Member Author

JKRhb commented Oct 13, 2021

Can you tell me what other issues exist and maybe fix them? I have also set up Github Actions, it seems to be working.

Awesome!

This PR looks good so far, it can be merged, right?

There are currently a number of tests which have to be skipped to make the CI green :/ When you remove the .skip from the tests you can see what the problem is.

I think it might be necessary to rework the setup of the argument parser a bit more and add the options to the subcommands rather than the top-level. I try have to have another look into it soon, feel free to make changes to the PR if you see a good solution before that :)

@JKRhb
Copy link
Member Author

JKRhb commented Oct 13, 2021

I rebased the PR against main, the tests which are not working should now be more visible :)

@gerdemann
Copy link
Member

Unfortunately, I can't help right now because I don't have time. It would be great if you could take care of it.

@JKRhb
Copy link
Member Author

JKRhb commented Oct 13, 2021

Unfortunately, I can't help right now because I don't have time. It would be great if you could take care of it.

Sure, will do :) I'll ping you once the PR is ready to be reviewed :)

@JKRhb
Copy link
Member Author

JKRhb commented Oct 17, 2021

@gerdemann This PR should now be ready to be merged :) Apart from the fact that you now need to explicitly state which method you are using (e. g. get) I was able to keep the general structure of the CLI so I only made minimal adjustments to the README. The IPv6 issue should now be fixed as well so merging this PR will also close #56.

@gerdemann
Copy link
Member

That looks great. Thank you very much!

@gerdemann gerdemann merged commit 5d1159b into coapjs:main Oct 18, 2021
@JKRhb JKRhb deleted the fix-commander branch October 18, 2021 05:32
@gerdemann
Copy link
Member

@JKRhb I published a new Version 0.8.0

@JKRhb
Copy link
Member Author

JKRhb commented Oct 18, 2021

@JKRhb I published a new Version 0.8.0

Awesome, thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants