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: remove sortBy prop default value #252

Merged
merged 1 commit into from
Dec 28, 2021

Conversation

Clebal
Copy link
Contributor

@Clebal Clebal commented Dec 27, 2021

Using [] as default sortBy value makes the execution to break due to it's an invalid sortBy value.

@Clebal Clebal requested review from alasarr and padawannn December 27, 2021 19:10
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 1628021541

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 68.913%

Totals Coverage Status
Change from base Build 1626881185: 0.0%
Covered Lines: 1044
Relevant Lines: 1424

💛 - Coveralls

@alasarr
Copy link
Contributor

alasarr commented Dec 28, 2021

Can you provide an example of the failure?

@Clebal
Copy link
Contributor Author

Clebal commented Dec 28, 2021

@alasarr

If you want to get the features of certain source with certain filters, but you don't want to sort the features:

executeTask(sourceId, Methods.FEATURES_RAW, { filters: sourceFilters })

It makes the code break, because in sorting.js we have a validation of sortBy prop which cannot be an empty array. If you don't want to sort by anything, then sortBy should be undefined.

@alasarr alasarr merged commit 5a2ef93 into master Dec 28, 2021
@alasarr alasarr changed the title Remove sortBy prop default value Fix: remove sortBy prop default value Dec 28, 2021
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