Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

field using types turned into array when declared(params) #2112

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

braktar
Copy link
Contributor

@braktar braktar commented Oct 5, 2020

It seems that the recent works on declared change the interaction between 'declared' and the 'types' field

@grape-bot
Copy link

grape-bot commented Oct 5, 2020

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#2112](https://github.com/ruby-grape/grape/pull/2112): Field using type turned into array when declared(params) - [@braktar](https://github.com/braktar).

Generated by 🚫 danger

@dblock
Copy link
Member

dblock commented Oct 5, 2020

what was the behavior in 1.3.x, 1.4.x?

either way looks like a bug, another one... /cc: @tlconnor

@dblock
Copy link
Member

dblock commented Oct 5, 2020

Related #2103

@braktar
Copy link
Contributor Author

braktar commented Oct 5, 2020

Before the integration of #2103 the spec passed successfully

@braktar
Copy link
Contributor Author

braktar commented Oct 5, 2020

Here is a fix which avoid the misinterpretation of the type

@braktar
Copy link
Contributor Author

braktar commented Oct 5, 2020

That's strange that the type of empty_typed_arr turned from Array[String] to [String] when arrived into handle_passed_param

@dblock
Copy link
Member

dblock commented Oct 5, 2020

Let's extend tests with Hash and Set along with Array?

if type == 'Hash' && !has_children
{}
elsif type == 'Array' || type&.start_with?('[')
elsif type == 'Array' || type && type.start_with?('[') && !type.include?(',')
Copy link
Contributor

Choose a reason for hiding this comment

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

You are right @dblock that working with Strings here is not ideal. I don't know enough to know if there is a better alternative.

@braktar
Copy link
Contributor Author

braktar commented Oct 6, 2020

Let's extend tests with Hash and Set along with Array?

I don't really see what should exhibit these tests, as the only ambiguity is between types : [x, y] and type: Array[x, y] which both give type: [x, y] while processed.

It seems to require a deeper investigation

@braktar braktar changed the title field using type turned into array when declared(params) field using types turned into array when declared(params) Oct 6, 2020
@dblock
Copy link
Member

dblock commented Oct 6, 2020

Let's extend tests with Hash and Set along with Array?

I don't really see what should exhibit these tests, as the only ambiguity is between types : [x, y] and type: Array[x, y] which both give type: [x, y] while processed.

It seems to require a deeper investigation

I mean what happens if requires :bar, type: Hash do ...?

@dblock
Copy link
Member

dblock commented Oct 9, 2020

@stanhu isn't this the same?

@stanhu
Copy link
Contributor

stanhu commented Oct 9, 2020

Yes, it's the same. I just picked the commit in #2117.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants