You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "hoslo (via GitHub)" <gi...@apache.org> on 2024/04/02 05:29:51 UTC
[PR] feat(services/github): support list recursive [opendal]
hoslo opened a new pull request, #4423:
URL: https://github.com/apache/opendal/pull/4423
Part #4253
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/github): support list recursive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4423:
URL: https://github.com/apache/opendal/pull/4423#discussion_r1578812209
##########
core/src/services/github/lister.rs:
##########
@@ -24,27 +24,34 @@ use crate::*;
pub struct GithubLister {
core: Arc<GithubCore>,
+
path: String,
+ recursive: bool,
}
impl GithubLister {
- pub fn new(core: Arc<GithubCore>, path: &str) -> Self {
+ pub fn new(core: Arc<GithubCore>, path: &str, recursive: bool) -> Self {
Self {
core,
path: path.to_string(),
+ recursive,
}
}
}
impl oio::PageList for GithubLister {
async fn next_page(&self, ctx: &mut oio::PageContext) -> Result<()> {
- let entries = self.core.list(&self.path).await?;
+ let resp = self.core.list(&self.path).await?;
+
+ // Record whether there is a dir in the list so that we need recursive later.
+ let mut has_dir = false;
- for entry in entries {
+ for entry in resp.entries {
Review Comment:
We can loop `resp.entries` first to check `has_dir` first before parse `Entry`.
##########
core/src/services/github/core.rs:
##########
@@ -259,7 +261,34 @@ impl GithubCore {
let resp: ListResponse =
serde_json::from_reader(body.reader()).map_err(new_json_deserialize_error)?;
- Ok(resp.entries)
+ Ok(resp)
+ }
+ StatusCode::NOT_FOUND => Ok(ListResponse::default()),
+ _ => Err(parse_error(resp).await?),
+ }
+ }
+
+ pub async fn list_with_recursive(&self, url: &str) -> Result<Vec<Tree>> {
Review Comment:
Please add comments for this API to make it clear that this API calls github's Tree based API that requires `git_url`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/github): support list recursive [opendal]
Posted by "hoslo (via GitHub)" <gi...@apache.org>.
hoslo commented on PR #4423:
URL: https://github.com/apache/opendal/pull/4423#issuecomment-2076278991
@Xuanwo Can this merge now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/github): support list recursive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4423:
URL: https://github.com/apache/opendal/pull/4423#discussion_r1579164466
##########
core/src/services/github/lister.rs:
##########
@@ -24,27 +24,34 @@ use crate::*;
pub struct GithubLister {
core: Arc<GithubCore>,
+
path: String,
+ recursive: bool,
}
impl GithubLister {
- pub fn new(core: Arc<GithubCore>, path: &str) -> Self {
+ pub fn new(core: Arc<GithubCore>, path: &str, recursive: bool) -> Self {
Self {
core,
path: path.to_string(),
+ recursive,
}
}
}
impl oio::PageList for GithubLister {
async fn next_page(&self, ctx: &mut oio::PageContext) -> Result<()> {
- let entries = self.core.list(&self.path).await?;
+ let resp = self.core.list(&self.path).await?;
+
+ // Record whether there is a dir in the list so that we need recursive later.
+ let mut has_dir = false;
- for entry in entries {
+ for entry in resp.entries {
Review Comment:
"iterate through resp.entries" is cheap, I care more about readability here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/github): support list recursive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4423:
URL: https://github.com/apache/opendal/pull/4423#discussion_r1580553597
##########
core/src/services/github/lister.rs:
##########
@@ -24,39 +24,84 @@ use crate::*;
pub struct GithubLister {
core: Arc<GithubCore>,
+
path: String,
+ recursive: bool,
}
impl GithubLister {
- pub fn new(core: Arc<GithubCore>, path: &str) -> Self {
+ pub fn new(core: Arc<GithubCore>, path: &str, recursive: bool) -> Self {
Self {
core,
path: path.to_string(),
+ recursive,
}
}
}
impl oio::PageList for GithubLister {
async fn next_page(&self, ctx: &mut oio::PageContext) -> Result<()> {
- let entries = self.core.list(&self.path).await?;
+ let resp = self.core.list(&self.path).await?;
+
+ // Record whether there is a dir in the list so that we need recursive later.
+ let mut has_dir = false;
+ for entry in &resp.entries {
Review Comment:
Please use `resp.entries.iter().any()`;
##########
core/src/services/github/lister.rs:
##########
@@ -24,39 +24,84 @@ use crate::*;
pub struct GithubLister {
core: Arc<GithubCore>,
+
path: String,
+ recursive: bool,
}
impl GithubLister {
- pub fn new(core: Arc<GithubCore>, path: &str) -> Self {
+ pub fn new(core: Arc<GithubCore>, path: &str, recursive: bool) -> Self {
Self {
core,
path: path.to_string(),
+ recursive,
}
}
}
impl oio::PageList for GithubLister {
async fn next_page(&self, ctx: &mut oio::PageContext) -> Result<()> {
- let entries = self.core.list(&self.path).await?;
+ let resp = self.core.list(&self.path).await?;
+
+ // Record whether there is a dir in the list so that we need recursive later.
+ let mut has_dir = false;
+ for entry in &resp.entries {
+ if entry.type_field == "dir" {
+ has_dir = true;
+ break;
+ }
+ }
- for entry in entries {
- let path = build_rel_path(&self.core.root, &entry.path);
- let entry = if entry.type_field == "dir" {
- let path = format!("{}/", path);
- Entry::new(&path, Metadata::new(EntryMode::DIR))
- } else {
- if path.ends_with(".gitkeep") {
- continue;
- }
- let m = Metadata::new(EntryMode::FILE)
- .with_content_length(entry.size)
- .with_etag(entry.sha);
- Entry::new(&path, m)
- };
+ // if recursive is true and there is a dir in the list, we need to list it recursively.
+ if self.recursive && has_dir {
Review Comment:
Please return early by checking `if !self.recursive || !has_dir`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/github): support list recursive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #4423:
URL: https://github.com/apache/opendal/pull/4423#issuecomment-2034090863
Hi, I'm working on a refactor that might conflict with this PR. Would you like to wait me for one week?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/github): support list recursive [opendal]
Posted by "hoslo (via GitHub)" <gi...@apache.org>.
hoslo commented on PR #4423:
URL: https://github.com/apache/opendal/pull/4423#issuecomment-2034145544
> Hi, I'm working on a refactor that might conflict with this PR. Would you like to wait me for one week?
Of course.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/github): support list recursive [opendal]
Posted by "hoslo (via GitHub)" <gi...@apache.org>.
hoslo commented on code in PR #4423:
URL: https://github.com/apache/opendal/pull/4423#discussion_r1579181140
##########
core/src/services/github/lister.rs:
##########
@@ -24,27 +24,34 @@ use crate::*;
pub struct GithubLister {
core: Arc<GithubCore>,
+
path: String,
+ recursive: bool,
}
impl GithubLister {
- pub fn new(core: Arc<GithubCore>, path: &str) -> Self {
+ pub fn new(core: Arc<GithubCore>, path: &str, recursive: bool) -> Self {
Self {
core,
path: path.to_string(),
+ recursive,
}
}
}
impl oio::PageList for GithubLister {
async fn next_page(&self, ctx: &mut oio::PageContext) -> Result<()> {
- let entries = self.core.list(&self.path).await?;
+ let resp = self.core.list(&self.path).await?;
+
+ // Record whether there is a dir in the list so that we need recursive later.
+ let mut has_dir = false;
- for entry in entries {
+ for entry in resp.entries {
Review Comment:
fixed
##########
core/src/services/github/core.rs:
##########
@@ -259,7 +261,34 @@ impl GithubCore {
let resp: ListResponse =
serde_json::from_reader(body.reader()).map_err(new_json_deserialize_error)?;
- Ok(resp.entries)
+ Ok(resp)
+ }
+ StatusCode::NOT_FOUND => Ok(ListResponse::default()),
+ _ => Err(parse_error(resp).await?),
+ }
+ }
+
+ pub async fn list_with_recursive(&self, url: &str) -> Result<Vec<Tree>> {
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/github): support list recursive [opendal]
Posted by "hoslo (via GitHub)" <gi...@apache.org>.
hoslo commented on code in PR #4423:
URL: https://github.com/apache/opendal/pull/4423#discussion_r1579128423
##########
core/src/services/github/lister.rs:
##########
@@ -24,27 +24,34 @@ use crate::*;
pub struct GithubLister {
core: Arc<GithubCore>,
+
path: String,
+ recursive: bool,
}
impl GithubLister {
- pub fn new(core: Arc<GithubCore>, path: &str) -> Self {
+ pub fn new(core: Arc<GithubCore>, path: &str, recursive: bool) -> Self {
Self {
core,
path: path.to_string(),
+ recursive,
}
}
}
impl oio::PageList for GithubLister {
async fn next_page(&self, ctx: &mut oio::PageContext) -> Result<()> {
- let entries = self.core.list(&self.path).await?;
+ let resp = self.core.list(&self.path).await?;
+
+ // Record whether there is a dir in the list so that we need recursive later.
+ let mut has_dir = false;
- for entry in entries {
+ for entry in resp.entries {
Review Comment:
The reason for this is that if dir doesn't exist, we don't need to iterate through resp.entries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/github): support list recursive [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #4423:
URL: https://github.com/apache/opendal/pull/4423
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org