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

Require user to supply --convert-to-dataset-format #845

Merged
merged 2 commits into from
May 24, 2023

Conversation

olsen232
Copy link
Collaborator

@olsen232 olsen232 commented May 22, 2023

... or, sometimes, --no-convert-to-dataset-format

--convert-to-dataset-format: the user acknowledges that the tiles as they are don't comply to the dataset format constraint. Kart is allowed to convert them during commit so that they do. In practise, the conversions will be any of:
make cloud optimized
change laz version (eg LAZ 1.2 to LAZ 1.4)

--no-convert-to-dataset-format: the user acknowledges that the tiles as they are don't comply to the dataset format constraint. Kart is allowed to update the dataset format so that they do. In practice the update will be:

  • drop cloud optimized constraint
  • change laz format of entire dataset (only allowed if every new tile conforms to the new format)

This also updates kart diff to better explain these type of errors: until now, they were shown as conflicts with <<<, >>> markers, but the associated error message isn't shown until you kart commit. But kart diff will now also show these warnings.

#842

Checklist:

  • Have you reviewed your own change?
  • Have you included test(s)?
  • Have you updated the changelog?

@olsen232 olsen232 requested review from craigds and fedch May 22, 2023 02:27
... or, sometimes, --no-convert-to-dataset-format

--convert-to-dataset-format: the user acknowledges that the tiles
as they are don't comply to the dataset format constraint. Kart
is allowed to convert them during commit so that they do.
In practise, the conversions will be any of:
make cloud optimized
change laz version (eg LAZ 1.2 to LAZ 1.4)

--no-convert-to-dataset-format: the user acknowledges that the
tiles as they are don't comply to the dataset format constraint.
Kart is allowed to update the dataset format so that they do.
In practice the update will be:
- drop cloud optimized constraint
- change laz format of entire dataset (only allowed if every new tile conforms
  to the new format)
self.write_pk_conflicts_warning_footer()
self.write_list_of_conflicts_warning_footer()

def write_pk_conflicts_warning_footer(self):
Copy link
Member

Choose a reason for hiding this comment

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

are 'PK conflicts' always 'spatial filter conflicts' and vice versa? I'd suggest renaming consistently to the latter (it's the most descriptive) rather than using two names

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

merged_format.error_message = "Committing LAS tiles is not supported, unless you specify the --convert-to-dataset-format flag"
merged_metadata["format.json"] = merged_format

def is_cloud_optimized(self):
return self.get_meta_item("format.json").get("optimization") == "copc"
def _ensure_list(self, arg):
Copy link
Member

Choose a reason for hiding this comment

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

unused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used in one place - I'll inline it to where it's used

@olsen232 olsen232 merged commit 28653aa into master May 24, 2023
@olsen232 olsen232 deleted the diff-commit-prompts branch May 24, 2023 07:46
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.

2 participants