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

Implement read_dir function and test #257

Merged
merged 5 commits into from May 13, 2022
Merged

Implement read_dir function and test #257

merged 5 commits into from May 13, 2022

Conversation

Barugon
Copy link
Contributor

@Barugon Barugon commented Feb 10, 2022

Implement a gdal::vsi::read_dir function and test.

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

@Barugon Barugon changed the title Implement read_dir funcion and test Implement read_dir function and test Feb 10, 2022
Copy link
Contributor

@ChristianBeilschmidt ChristianBeilschmidt left a comment

Choose a reason for hiding this comment

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

I've got some comments.

src/vsi.rs Outdated Show resolved Hide resolved
src/vsi.rs Outdated Show resolved Hide resolved
src/vsi.rs Outdated Show resolved Hide resolved
src/raster/tests.rs Outdated Show resolved Hide resolved
src/raster/tests.rs Outdated Show resolved Hide resolved
@ChristianBeilschmidt
Copy link
Contributor

👍

Can someone, e.g. @jdroenner, activate the CI?

@Barugon
Copy link
Contributor Author

Barugon commented Feb 24, 2022

I just realized that the test should be in vsi.rs. Fixing....

@Barugon
Copy link
Contributor Author

Barugon commented Apr 11, 2022

Could you please activate CI again for this branch?

@lnicola
Copy link
Member

lnicola commented Apr 12, 2022

We can't activate it directly, it's disabled for PRs in the workflow, and some convincing might be required to change that.

But at least we can

bors try

bors bot added a commit that referenced this pull request Apr 12, 2022
@bors
Copy link
Contributor

bors bot commented Apr 12, 2022

try

Build failed:

@lnicola
Copy link
Member

lnicola commented Apr 12, 2022

CC #266

@lnicola
Copy link
Member

lnicola commented Apr 12, 2022

Filed #267 for the clippy error.

src/vsi.rs Outdated Show resolved Hide resolved
src/vsi.rs Outdated Show resolved Hide resolved
@lnicola
Copy link
Member

lnicola commented Apr 12, 2022

Hmm, CI works now.

@Barugon
Copy link
Contributor Author

Barugon commented Apr 12, 2022

Hmm, CI works now.

We'll need your change for it to not fail though.

@lnicola
Copy link
Member

lnicola commented Apr 12, 2022

Yeah, I'm just confused because the last time it didn't even show up on the page, which made me file #266.

bors bot added a commit that referenced this pull request Apr 20, 2022
267: Mark `SpatialRef::from_c_obj` as unsafe r=jdroenner a=lnicola

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

See #257 (comment)

Co-authored-by: Laurențiu Nicola <lnicola@dend.ro>
@lnicola
Copy link
Member

lnicola commented Apr 20, 2022

Can you rebase?

@Barugon
Copy link
Contributor Author

Barugon commented Apr 20, 2022

I was having trouble rebasing, so I merged. Should be good though.

@lnicola
Copy link
Member

lnicola commented May 13, 2022

Oof, sorry, I missed your comment. Can you resolve the conflict, then maybe squash? It's fine if you don't, though.

@Barugon
Copy link
Contributor Author

Barugon commented May 13, 2022

Hopefully I did that correctly.

@lnicola
Copy link
Member

lnicola commented May 13, 2022

bors r+

bors bot added a commit that referenced this pull request May 13, 2022
257: Implement read_dir function and test r=lnicola a=Barugon

Implement a `gdal::vsi::read_dir` function and test.

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---



Co-authored-by: Barugon <barugon@dungeonbox.net>
@lnicola
Copy link
Member

lnicola commented May 13, 2022

Hopefully I did that correctly.

Almost, except the commit message 😅.

@lnicola
Copy link
Member

lnicola commented May 13, 2022

bors r-

@bors
Copy link
Contributor

bors bot commented May 13, 2022

Canceled.

@lnicola
Copy link
Member

lnicola commented May 13, 2022

Sorry, I missed this, @ChristianBeilschmidt had a good question up there.

@Barugon
Copy link
Contributor Author

Barugon commented May 13, 2022

Sorry, I missed this, @ChristianBeilschmidt had a good question up there.

Which comment? AFAIK, everything was resolved.

@lnicola
Copy link
Member

lnicola commented May 13, 2022

Still unresolved:

image

The return type, which has String instead of PathBuf.

src/vsi.rs Outdated
@@ -28,7 +28,14 @@ fn _read_dir(path: &Path, recursive: bool) -> Result<Vec<String>> {
data
};

Ok(_string_array(data))
let strings = _string_array(data);
Copy link
Member

Choose a reason for hiding this comment

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

We can do it in a later PR, but you could add _pathbuf and _pathbuf_array functions to avoid the reallocation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's getting late here. Let me do that tomorrow.

@ChristianBeilschmidt
Copy link
Contributor

I'm sorry if I blocked this. I guess my comments were resolved.
@lnicola has a comment open and can resolve this then.

src/utils.rs Outdated
where
F: Fn(*const c_char) -> R,
{
let mut ret_val: Vec<R> = vec![];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason that this couldn't just be let mut ret_val = Vec::new();?

Copy link
Member

Choose a reason for hiding this comment

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

Probably not

@lnicola
Copy link
Member

lnicola commented May 13, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented May 13, 2022

Build succeeded:

@bors bors bot merged commit b0ebd1d into georust:master May 13, 2022
@Barugon Barugon deleted the vsi_read_dir branch May 13, 2022 17:44
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.

None yet

3 participants