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

Getters for underlying libraries and setters for data fields #17

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

tormit
Copy link
Contributor

@tormit tormit commented May 21, 2024

Constants and setters for data fields based on specification https://qr-platba.cz/pro-vyvojare/specifikace-formatu/.
Basic data sanitation.
Getters for underlying Spayd and QR code builder to be able to modify certain elements.

…are/specifikace-formatu/); Basic data sanitation; Getter for Spayd; Getter for underlying QR code builder
@tormit tormit changed the title Getters for undelying libraries and setters for data fields Getters for underlying libraries and setters for data fields May 21, 2024
@petrknap
Copy link
Owner

Thanks for contribution, I will look at it soon. :)

@petrknap
Copy link
Owner

How much of a rush are you in? It's going to need unit tests at the very least, and looking at the whole thing, I'd probably refactor it. If you are in a hurry I'd release it when the tests are added, if you're not in a hurry I'll refactor it first.

@tormit
Copy link
Contributor Author

tormit commented May 22, 2024

I could add some tests. What would you refactor? Please don't break my code :)

@petrknap
Copy link
Owner

petrknap commented May 28, 2024

I did just small refactoring for current version, but I'm working on big refactor which I will release as major update. Add test for the sanitizeString method as it is modifying input data then I will release it as last update of current version. In next version I will replace it by SpaydValue object.

@tormit
Copy link
Contributor Author

tormit commented May 29, 2024

I moved the sanitation to new utility class to test it easily and make main class cleaner.
Also added the tests. Not much to test though.

@tormit
Copy link
Contributor Author

tormit commented May 29, 2024

Also I would suggest to improve the sanitation by transliterating non ISO-8859-1 symbols and striping special characters.
On my implementation level, I use voku UTF8 library for that. Here's example:

private function sanitize(string $value)
    {
        return \voku\helper\UTF8::to_filename(mb_strtoupper(\voku\helper\UTF8::to_ascii($value), 'UTF-8'), false, ' ');
    }

Not sure you if you like additional dependencies in your project.
I, however, would gladly move this also down to your package level.

@petrknap petrknap changed the base branch from main to v3.x June 1, 2024 07:27
@petrknap
Copy link
Owner

petrknap commented Jun 1, 2024

I created a new branch for v3, so if you wish, you can add it there. I will release it as next v3 release (if it will be backward compatible) and refactor it to v4 (if it will make sense).

@petrknap
Copy link
Owner

petrknap commented Jun 1, 2024

FILE: /app/src/Util.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 17 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------

@petrknap
Copy link
Owner

petrknap commented Jun 1, 2024

Note me here when it will be ready and I will look at it once again and merge it. :)

@tormit
Copy link
Contributor Author

tormit commented Jun 1, 2024

I think let's leave the improved sanitation to v4. Adding additional library could cause version conflicts for someone.

@petrknap
Copy link
Owner

petrknap commented Jun 1, 2024

So is this ready to review/merge? :)

BTW I don't think that changing requirements is an issue - these who will have conflict will be able to install previous version (and also following version in this case).

@tormit
Copy link
Contributor Author

tormit commented Jun 1, 2024 via email

@petrknap petrknap merged commit a8854b0 into petrknap:v3.x Jun 4, 2024
2 checks passed
@petrknap petrknap linked an issue Jun 18, 2024 that may be closed by this pull request
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.

Make QR code builder accessible via getter
2 participants