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

core: Merge scan into list #2072

Closed
2 of 4 tasks
Xuanwo opened this issue Apr 23, 2023 · 4 comments · Fixed by #2243
Closed
2 of 4 tasks

core: Merge scan into list #2072

Xuanwo opened this issue Apr 23, 2023 · 4 comments · Fixed by #2243
Assignees
Labels
core help wanted Extra attention is needed

Comments

@Xuanwo
Copy link
Member

Xuanwo commented Apr 23, 2023

Background

We used to split scan and list to represent list without delimiter and list with delimiter / for easier implement. However, after capability implemented, this split is not needed anymore. We have to repeat work between scan and list because they nearly share the same args. For example, #2063

Thus, it's better for us to merge scan into list.

Actions

  • Add delimiter in OpList (default to /)
  • Add list_without_delimiter and list_with_delimiter_slash in Capability
  • Implement scan feature in list
  • Remove scan API (keep Operator::scan as an alias of Operator::list_with(OpList::with_delimiter("")))

Notes

  • No public API will be changed.
  • This issue is a good second issues that suitable for developer who want to contribuote more in OpenDAL.
@Xuanwo Xuanwo added help wanted Extra attention is needed core labels Apr 23, 2023
@cuichenli
Copy link
Contributor

hi @Xuanwo , i would like to work on this if it is ok

@Xuanwo
Copy link
Member Author

Xuanwo commented Apr 23, 2023

hi @Xuanwo , i would like to work on this if it is ok

Of course! Have fun!

@cuichenli
Copy link
Contributor

The PR I raised #2214 has been merged (forgot to mention this issue in the pr 🤦 ). Guess this issue can be closed now.

@Xuanwo
Copy link
Member Author

Xuanwo commented May 8, 2023

The PR I raised #2214 has been merged (forgot to mention this issue in the pr facepalm ). Guess this issue can be closed now.

We should remove the Accessor::scan entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants