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

Added the possibility to send an empty json list in rest steps #334

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

romcooo
Copy link
Contributor

@romcooo romcooo commented Mar 27, 2020

Added the possibility to send an empty json list in rest steps and fixed 3 SonarLint warnings.
Solves #326

Copy link
Contributor

@paveljandejsek paveljandejsek left a comment

Choose a reason for hiding this comment

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

Nice first PR, good work!

Couple of thoughts from my side:

  • I know that it was like that in the previous versions and is the same with the other pattern - but are we able to replace this with something like indexedKeyPattern3.pattern() + ".*" to avoid repetition of the same pattern again? Not saying to do it, just wondering if its possible or if there is some problem I am not seeing at first glance. Also if possible I would probably rename the indexedKeyPattern* variables to something more readable (again not saying to do it in this PR, but might be worth creating issue/another PR for it)
  • This solution supports only the [] notation for the array in examples table not the . one, are we okay with that? (I dont have a clue how we would even write it for the dot case, just want to point it out that this is the case).
  • Can you add some example to the markdown documentation as well?

@romcooo
Copy link
Contributor Author

romcooo commented Mar 28, 2020

Nice first PR, good work!

Couple of thoughts from my side:

* I know that it was like that in the previous versions and is the same with the other pattern - but are we able to replace this with something like `indexedKeyPattern3.pattern() + ".*"` to avoid repetition of the same pattern again? Not saying to do it, just wondering if its possible or if there is some problem I am not seeing at first glance. Also if possible I would probably rename the `indexedKeyPattern*` variables to something more readable (again not saying to do it in this PR, but might be worth creating issue/another PR for it)

* This solution supports only the `[]` notation for the array in examples table not the `.` one, are we okay with that? (I dont have a clue how we would even write it for the dot case, just want to point it out that this is the case).

* Can you add some example to the markdown documentation as well?

Thanks for the feedback :) In order:

  1. I agree that the patterns could probably be made more clear, but I was a bit scared to make these changes in my first PR :) But I can take a better look at this on Monday.
  2. Honestly, I forgot about the dot notation for lists, sorry about that. I think a notation such as "documents." could indicate an empty list, but it seems a bit less self-explanatory than "documents[]". I personally think that it would be acceptable to have only the "[]" notation support empty lists, but if you or others think that it would be better to have support in both, I can try to add it as well.
  3. I documented the new functionality in the Rest-api.md file and amended my previous commit - please, take a look and let me know if it's sufficient.

@paveljandejsek
Copy link
Contributor

  1. I agree that the patterns could probably be made more clear, but I was a bit scared to make these changes in my first PR :) But I can take a better look at this on Monday.

Dont do them just yet, I am just wondering if its possible for some future work, or if what I said is total BS 🙂

  1. Honestly, I forgot about the dot notation for lists, sorry about that. I think a notation such as "documents." could indicate an empty list, but it seems a bit less self-explanatory than "documents[]". I personally think that it would be acceptable to have only the "[]" notation support empty lists, but if you or others think that it would be better to have support in both, I can try to add it as well.

I dont know myself either, I cant think of any "nice" way to write it with dot notation, but I am personally fine without it - so I would maybe leave this point open till wednesday when hopefully more people will take a look at it.

  1. I documented the new functionality in the Rest-api.md file and amended my previous commit - please, take a look and let me know if it's sufficient.

Awesome, approved by me 👍

@thradec thradec merged commit e4a0731 into EmbedITCZ:master Apr 1, 2020
@romcooo romcooo deleted the restStepsEmptyList branch June 18, 2020 08:53
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.

3 participants