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

Generalize TileSingleImage #761

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Generalize TileSingleImage #761

wants to merge 13 commits into from

Conversation

mtiessen1175
Copy link
Collaborator

@mtiessen1175 mtiessen1175 commented Jan 26, 2024

Made TileSingleImage-class more general to be used with other formats such as GeoOverlays.

This PR is associated with PR #47 from biigle/geo.

@mtiessen1175 mtiessen1175 self-assigned this Jan 29, 2024
@mtiessen1175 mtiessen1175 marked this pull request as ready for review January 29, 2024 13:47
app/Jobs/TileSingleImage.php Outdated Show resolved Hide resolved
app/Jobs/TileSingleImage.php Outdated Show resolved Hide resolved
app/Jobs/TileSingleImage.php Outdated Show resolved Hide resolved
Copy link
Member

@mzur mzur 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. There is only a minor comment left.

I should have a look why Psalm is complaining about the missing class.

app/Jobs/MigrateTiledImage.php Outdated Show resolved Hide resolved
app/Jobs/TileSingleImage.php Outdated Show resolved Hide resolved
@mtiessen1175 mtiessen1175 requested a review from mzur July 18, 2024 11:58
mzur
mzur previously approved these changes Jul 19, 2024
Copy link
Member

@mzur mzur 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. This belongs to a new feature of biigle/geo, right? I'll wait until it is ready then and the PRs can be merged together.

@mzur
Copy link
Member

mzur commented Sep 9, 2024

@mtiessen1175 Can you please resolve the conflicts of this branch?

@mtiessen1175
Copy link
Collaborator Author

mtiessen1175 commented Sep 12, 2024

@mzur, since having merged the master-branch into this one locally, I get a few strange errors (such as the one described here) when trying to execute tests or even do artisan commands. I guess this has to do with the upgrade to Laravel 11 (as done here)? Could I safely upgrade my local instance (which currently is v.10.48.4)? Or do I have to be aware of anything that I should do before upgrading?

update: I upgraded the dependencies on my local instance using composer update --ignore-platform-reqs while on the dev-modules branch. Subsequently, I ran the php artisan config:clear and php artisan cache:clear commands. Unfortunately, I cannot execute the app due to some Symfony Exceptions. Is there any other required step to get the application up and running again after upgrading the dependencies?

update: I believe that the problem comes from using outdated docker images. I checked the php version on the app-image with docker compose exec app php --version and it currently uses PHP 8.1.3. Laravel 11 requires at least 8.2.0, so I guess an update of the images should do.

update: Solved by updating to the latest docker images.

@mtiessen1175
Copy link
Collaborator Author

@mzur, the php-linter still complains about accessing undefined properties, such as Biigle\FileCache\Contracts\File::$uuid. There are several ways to solve this type of error, as explained here (e.g. declaring the properties in the class docs with @property string $uuid). The linter also complains about Laravel eloquent model attributes such as Biigle\FileCache\Contracts\File::save(), which I find strange.

How would you approach this error? In the case at hand with the TileSingleImage and TileSingleOverlay classes, narrowing the type the property is accessed on could be one solution. I am thinking of creating an abstract class or interface TileSingleObject and then specify the type (Image, Overlay, etc.) in its child classes instead of using the more general FileCache type.

@mzur
Copy link
Member

mzur commented Sep 16, 2024

I'll have a look at the linter errors. If they are genuine, I'll suggest a fix and if not, I'll try to update the linter settings. The linter has become a little more strict since the Laravel 11 upgrade.

@mzur
Copy link
Member

mzur commented Sep 19, 2024

The linter errors are genuine. We have three errors:

  1. Access to an undefined property Biigle\FileCache\Contracts\File::$uuid: This is true, as the File interface does not specify a UUID. The Biigle\Image has a UUID but not the Biigle\Modules\Geo\GeoOverlay. So this would actually result in an incorrect tempPath for a GeoOverlay (if you wouldn't override this in TileSingleOverlay).

  2. Access to an undefined property Biigle\FileCache\Contracts\File::$tilingInProgress: Also correct. A GeoOverlay does not have this attribute. The job fails because of this.

  3. Call to an undefined method Biigle\FileCache\Contracts\File::save(): Also true. The File here is no Eloquent model and does not define this method.

To fix these errors I suggest that you implement a more abstract version of TileSingleImage. Then implement one special version to process Biigle\Image (with uuid for the tempPath and tilingInProgress and save()) and in biigle/geo keep TileSingleOverlay (with id for the tempPath and without tilingInProgress and save()).

@mtiessen1175
Copy link
Collaborator Author

I'll have a look at the linter errors. If they are genuine, I'll suggest a fix [...]

resolved the linter errors aa91823

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