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 json output #9067

Merged
merged 6 commits into from
May 1, 2023
Merged

Fix json output #9067

merged 6 commits into from
May 1, 2023

Conversation

vitormattos
Copy link
Contributor

@vitormattos vitormattos commented Mar 15, 2023

🚧 TODO

🏁 Checklist

@vitormattos
Copy link
Contributor Author

/backport to stable26

@nickvergessen
Copy link
Member

  • Tests (unit and/or integration) are included

Please add integration tests in https://github.com/nextcloud/spreed/tree/master/tests/integration/features/command

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

See comment

@nickvergessen nickvergessen marked this pull request as draft April 3, 2023 21:21
#8404

Signed-off-by: Vitor Mattos <vitor@php.rio>
@vitormattos
Copy link
Contributor Author

vitormattos commented Apr 25, 2023

@nickvergessen writing the integrations tests I identified that the command to delete a command is only possible delete using the command id not the command name. I think that would be good implement an option to make possible delete by command name and turn the argument of command id optional to don't break the current input of this command.

This small change will make the test more easy because we don't will need to use the output of add command to run the delete command. Can I implement this at this PR?

https://github.com/nextcloud/spreed/blob/master/lib/Command/Command/Delete.php#L42-L48

@nickvergessen
Copy link
Member

Integration tests for add/delete can be done later (by raising a follow up ticket).
But this PR should add one for the list command, so when someone reverts the fix we have a failing test

Verified only if the output contain a common json simbols: list of
objects: [{

Signed-off-by: Vitor Mattos <vitor@php.rio>
@vitormattos vitormattos force-pushed the bugfix/8404/fix-json-output branch from d9063d8 to cb537d6 Compare April 25, 2023 15:25
@vitormattos vitormattos marked this pull request as ready for review April 25, 2023 15:25
Signed-off-by: Vitor Mattos <vitor@php.rio>
Will be a bit complex to match a string with quote because the regex
ignore quotes: ([^"]*). If we change the regex don't will be possible
found the end of string and maybe we will need to change the delimiter
of the json string:

https://stackoverflow.com/questions/8533943/cucumber-fill-in-a-field-with-double-quote-in-it#8534220

I changed to accept the argument also as PyStringNode and don't need do
nothin more inside the method because the PyStringNode class have a
magich methdo __toString().

Signed-off-by: Vitor Mattos <vitor@php.rio>
…string

* Make the definition of steps more specific

Signed-off-by: Vitor Mattos <vitor@php.rio>
Signed-off-by: Vitor Mattos <vitor@php.rio>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants