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

Provide Search GraphQL Interfaces #150

Merged
merged 7 commits into from Nov 15, 2019

Conversation

@lifedraft
Copy link
Contributor

lifedraft commented Nov 12, 2019

No description provided.

search/interfaces/graphql/schema.graphql Outdated Show resolved Hide resolved
search/interfaces/graphql/schema.graphql Outdated Show resolved Hide resolved
danielpoe and others added 3 commits Nov 13, 2019
nicole.schmelzer
nicole.schmelzer
Copy link
Contributor

jeinfeldt left a comment

Thanks for contributing!

search/interfaces/graphql/dto/dto.go Outdated Show resolved Hide resolved
Copy link
Contributor

thehl left a comment

Thanks for contributing

search/domain/service.go Outdated Show resolved Hide resolved
search/domain/service.go Outdated Show resolved Hide resolved
SelectedDesc bool
// Asc - DEPRECATED - used to give the "Field" that is used to trigger ascending search

This comment has been minimized.

Copy link
@panzerfahrer

panzerfahrer Nov 15, 2019

Contributor

Maybe document the deprecation like: https://github.com/golang/go/wiki/Deprecated

The field seems completely unused. Could this potentially lead to undesired behavior for library consumers since the value is ignored? (same for Desc)

This comment has been minimized.

Copy link
@kalypso-byte

kalypso-byte Nov 15, 2019

I updated the deprecated fields. Which fields do you mean? Field or Asc/Desc? We left Asc/Desc there because we weren't sure if we introduce a breaking change if we remove them completely. In case of Field. We are using it for the graphql part and it is used in flamingo-om3 which is in the other merge request.

This comment has been minimized.

Copy link
@panzerfahrer

panzerfahrer Nov 15, 2019

Contributor

@danielpoe could both fields ( Asc, Desc) actually be deleted? this looks quite like dead code.

This comment has been minimized.

Copy link
@danielpoe

danielpoe Nov 18, 2019

Member

How do you know they are not used??
They are part of the public API - projects depend on it and its also filled in the Adapters currently
@panzerfahrer

This comment has been minimized.

Copy link
@panzerfahrer

panzerfahrer Nov 18, 2019

Contributor

The team decided to deprecate the field. If the fields are actually in use, we should probably add fallback behavior. Or do we need a rollback with another solution?

This comment has been minimized.

Copy link
@panzerfahrer

panzerfahrer Nov 18, 2019

Contributor

... @kalypso-byte could you please clarify, whether the deprecation was a valid move.

This comment has been minimized.

Copy link
@danielpoe

danielpoe Nov 18, 2019

Member

The DEPRECATED is valid we did this together. - but because the intent of this property is wrong (it represents the fieldname for searchperience) we want to remove it in future

We just need time to adjust the current usage

This comment has been minimized.

Copy link
@danielpoe

danielpoe Nov 18, 2019

Member

So all is fine at the moment - just wanted to clarify

nicole.schmelzer
@panzerfahrer panzerfahrer merged commit 3f98fe8 into master Nov 15, 2019
3 checks passed
3 checks passed
GolangCI No issues found!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
SelectedDesc bool
// Asc - represents the field that is used to trigger ascending search.
// Deprecated: use "Field" and "SelectedAsc" instead to set which field should be sortable

This comment has been minimized.

Copy link
@danielpoe

danielpoe Nov 18, 2019

Member

The message is wrong
It should say: Use "Field" to know which Field to set in the SortFilter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.