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

Added raw values of attributes to JSON API #988

Merged
merged 1 commit into from
May 2, 2021
Merged

Added raw values of attributes to JSON API #988

merged 1 commit into from
May 2, 2021

Conversation

nagmat84
Copy link
Collaborator

@nagmat84 nagmat84 commented May 2, 2021

The raw values are required by other clients than the web-front end, e.g. an android client which is currently under development

@codecov
Copy link

codecov bot commented May 2, 2021

Codecov Report

Merging #988 (d5d3f54) into master (82d1081) will decrease coverage by 1.56%.
The diff coverage is 100.00%.

@sonarcloud
Copy link

sonarcloud bot commented May 2, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ildyria ildyria merged commit c551fb1 into LycheeOrg:master May 2, 2021
@ildyria
Copy link
Member

ildyria commented May 2, 2021

// Maybe, it would be reasonable to migrate the web frontend to use the raw formats, too

Actually, I plan to nuke completely the full JS front-end. :)

@@ -24,7 +24,9 @@ public function toReturnArray(): array
'public' => $this->get_public(),
'album' => $this->album_id !== null ? strval($this->album_id) : null,
'width' => strval($this->width),
'width_raw' => $this->width !== null ? $this->width : -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for a late feedback, but I don't understand the value of width_raw and height_raw? They will contain exactly the same data as width and height, only as integers rather than strings, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. The main reason for these changes and other PRs (see PR #991 and PR #990) is to please clients (such as Androids Retrofit library) which insist on strong and correct typing. At the same time I didn't want to spend too much time on changing the current web frontend. As @ildyria mentioned he wants to nuke and rewrite it anyway. So I thought the most efficient way would be

  1. Add new attributes (with the correct type) to the JSON response even if this means that information is transported redundantly
    • This way I don't need to touch the current web frontend
    • I can concentrate on proceeding with the Android client
  2. Let @ildyria do the rewriting of the (new?) web frontend and hope that he will also use the "new" attributes
  3. Remove the old, redundant attributes in the future if they are not used any more by the web frontend

Copy link
Member

Choose a reason for hiding this comment

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

Let @ildyria do the rewriting of the (new?) web frontend and hope that he will also use the "new" attributes

I'm sorry to disappoint you but I will not use the REST api at all for the new front-end.

I wouldn't say that the JS one is depreciated because what I have is no-where near good enough to replace it yet. But in the end the REST api would only be useful for people using e.g. your Android client.

Copy link
Collaborator Author

@nagmat84 nagmat84 May 3, 2021

Choose a reason for hiding this comment

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

I'm sorry to disappoint you but I will not use the REST api at all for the new front-end.

No disappointment, but the opposite. Even better I would say. If the new frontend won't use the REST API, all "legacy" attributes will be able to be removed after that. Then, only the "raw" attributes which are a thin wrapper around the database can remain.

I wouldn't say that the JS one is depreciated because what I have is no-where near good enough to replace it yet.

@ildyria Do you have an rough estimate when you will have finished that? Are we talking about one or two months or more than a year? IMHO, an API should normally be kept clean with no redundancy. I only took that approach (add new attributes, keep the old attributes in place and don't touch them), because I thought this would only last a short term, intermediate period until the new web frontend is finished. However, if you say that it will probably last more then a short time, then I would also take the effort and have a look at the current front end and patch that to use the new attributes such that those old attributes can be removed from the API sooner than later.

But in the end the REST api would only be useful for people using e.g. your Android client.

No problem, see above

Copy link
Member

@ildyria ildyria May 3, 2021

Choose a reason for hiding this comment

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

I do expect it to take me 3 months if I were to work on it all my weekends. :'D
Because I basically need to rewrite everything in such way that visually it is similar to the JS front-end.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wow, from my perspective that's amazingly fast. Anyway, I will pay more attention to also align the current web front to the changes of the REST API such that the REST API remains clean and does not become cluttered with duplicate attributes.

@nagmat84 nagmat84 deleted the add-raw-values-to-api branch May 12, 2021 17:17
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