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

factor out layer copying into its own function #382

Merged
merged 4 commits into from
Jun 14, 2023

Conversation

maxfreu
Copy link
Contributor

@maxfreu maxfreu commented Jun 9, 2023

Hi! I have factored out the layer copying from writelayers into its own function. So basically I changed (pseudocode):

function writelayers()
  create() do
    copystuff
  end
end

to

function writelayers()
  create() do
    copylayers(src, dst)
  end
end

function copylayers(src, dst, ...)
  copystuff
end

That should now allow to copy data from and to arbitrary datasets, whether on disk or not. You can also give the layer indices you want to be copied. I added a tiny test for that. Furthermore I adjusted the docstrings for nicer rendering in the REPL. I'm not sure with the naming copylayers, is it ok? What do you think in general? Lastly, this supersedes #381, but I can take that part out if you want to merge this before the change to GDAL.

Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

Overall LGTM (modulo the changes requested), thank you!

src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Show resolved Hide resolved
src/dataset.jl Outdated Show resolved Hide resolved
src/dataset.jl Show resolved Hide resolved
src/dataset.jl Show resolved Hide resolved
@maxfreu
Copy link
Contributor Author

maxfreu commented Jun 12, 2023

I've just made the requested changes except for one :)

yeesian
yeesian previously approved these changes Jun 13, 2023
Copy link
Owner

@yeesian yeesian left a comment

Choose a reason for hiding this comment

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

@visr can I get your review here since it supersedes #381?

@maxfreu
Copy link
Contributor Author

maxfreu commented Jun 13, 2023

I just removed the line superseding #381, as it's just a minor change anyway. Now it can be merged regardless of the changes in GDAL itself.

visr
visr previously approved these changes Jun 13, 2023
Copy link
Collaborator

@visr visr left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@maxfreu
Copy link
Contributor Author

maxfreu commented Jun 13, 2023

Oh I just saw that the GDAL PR had already been merged and that you closed #381. I'll put the line back tomorrow, then this can be merged.

@maxfreu
Copy link
Contributor Author

maxfreu commented Jun 14, 2023

Ok, done - sorry for the forth and back!

@visr visr merged commit 0eb1f4d into yeesian:master Jun 14, 2023
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