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

refactor: changed from alllowercase to kebab-case for metadata keys #4695

Merged
merged 4 commits into from
May 24, 2024

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented May 23, 2024

Changed from alllowercase to kebab-case for metadata keys in propfind responses.

Otherwise it's impossible to convert to a different case in an automated way.

Otherwise it's impossible to convert to a different case in an automated
way.
@kulmann kulmann requested review from labkode, a team and glpatcern as code owners May 23, 2024 07:33
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Are we sure we can change that? Wouldn't that break backwards compatibility?

changelog/unreleased/add-photo-and-image-props.md Outdated Show resolved Hide resolved
@kulmann
Copy link
Member Author

kulmann commented May 23, 2024

Are we sure we can change that? Wouldn't that break backwards compatibility?

It's only for the metadata that @dschmidt and I introduced (audio, location, image, photo) and that is unused so far. But yes, it's a breaking change... don't know how to continue here. At least for image and photo the code was never released, so it should be safe to change that. audio and location have been in a release before though, so meh...

@@ -1147,19 +1148,11 @@ func mdToPropResponse(ctx context.Context, pf *XML, md *provider.ResourceInfo, p
appendMetadataProp := func(metadata map[string]string, tagNamespace string, name string, metadataPrefix string, keys []string) {
content := strings.Builder{}
for _, key := range keys {
lowerCaseKey := strings.ToLower(key)
kebabCaseKey := strcase.ToKebab(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

The keys are hardcoded above. Why even lower or kebap case them? IMO isVariableBitrate should just remain camel case. Any reasons against that?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kobergj wanted to achieve a somewhat consistent format for keys in the propfind response. From a client perspective that is a good thing to achieve. A mix of kebab, camel and snake case in the keys is annoying as hell...

However, my strongest need is, that I can convert the keys easily to a different case so that they match an interface of mine. Types/interfaces in web are all camelCase, so I need to be able to convert as needed.

@kulmann kulmann requested a review from kobergj May 23, 2024 14:47
@kulmann kulmann mentioned this pull request May 23, 2024
10 tasks
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Only thing I'm concerned about is backwards compatibility. Let's maybe not backport it to 5.0 to be safe?

@kulmann kulmann merged commit 7d17b75 into cs3org:edge May 24, 2024
9 checks passed
@micbar micbar mentioned this pull request Jun 19, 2024
24 tasks
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.

4 participants