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

Replace Haste\Units (Adaption to Contao5 Step 008) #2501

Closed
wants to merge 19 commits into from

Conversation

Ernestopheles
Copy link
Contributor

Replaced by jordanbrauer\unit-converter.

Fixed property type in 3 files.

Why draft?

  1. Draft as PHPUnit Tests not yet written. Tested with Cypress with some simple usecases using the Isotope Demo. Includes only few usecases! More tests neccessary!!

  2. Draft also as unsure if this solution fits in the projects concepts.

  3. Draft also as being unhappy with not working DI (needed to use container->get).

  4. Draft also as used PHP 8 typografy in constructors; therefore composer.json should reflect that (not yet included!)

@aschempp
Copy link
Member

I think the code pattern you should look at to fix the dependency injection issue is a "factory". Not sure if we need a ScaleFactory that creates new instanced of scales and a WeightFactory or maybe something combined. I would also not create services for all classes of the UnitConverter, I don't see any problem in hardcoding that in a factory method.

@Ernestopheles
Copy link
Contributor Author

Ernestopheles commented Nov 13, 2023

Worked your advices in. Factory working, thanks for that advice! DI not working again. But solution working as should nethertheless.

If you want this solution, let me know and I will add PHPUnit tests for the new classes.

Allso let me know if the mass units that had been in Haste but are not in jordanbrauer should be added.

Remember I used PHP 8 notation in constructor.

@Ernestopheles Ernestopheles marked this pull request as ready for review November 17, 2023 22:14
@Ernestopheles
Copy link
Contributor Author

Is there anything left the project is waiting for?

@aschempp
Copy link
Member

aschempp commented Dec 6, 2023

mostly time on my side 😉

@Ernestopheles
Copy link
Contributor Author

So I am left here without answer. I suppose my work is not valuable for you?

@aschempp
Copy link
Member

aschempp commented Dec 6, 2023

This is Open Source software, you have no right to expect anything from anyone. Noone said your work is not valuable, but my private time is equally valuable to me. https://www.terminal42.ch/de/open-source

@Ernestopheles
Copy link
Contributor Author

Stopped working on adaption Isotope to Contao5 as I do not see any basis for cooperation with the project any more. What a pity!

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