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

Updates to CI and code cleanup #88

Merged
merged 24 commits into from
May 22, 2024

Conversation

martijn00
Copy link
Contributor

@martijn00 martijn00 commented May 8, 2024

Solution description

Closes:

I've used this opportunity to also cleanup some code and CI. Let me know if i need to change anything.

It might be necessary to run flutter gen-l10n as a build step, but i can validate that once you approve running this PR.

Screenshots or Videos

To Do

  • Read contributing guide
  • Check the original issue to confirm it is fully satisfied
  • Add solution description to help guide reviewers
  • Add unit test to verify new or fixed behaviour
  • If apply, add documentation to code properties and package readme

@deandreamatias
Copy link
Contributor

deandreamatias commented May 12, 2024

Thanks a lot for contribution! Is something really necessary.

About the PR, the intl generated files are necessary because this package has embedded the translation of validations. So, I think that is necessary not ignore the generated files from git.

@deandreamatias deandreamatias self-requested a review May 12, 2024 18:44
pubspec.yaml Outdated Show resolved Hide resolved
@martijn00
Copy link
Contributor Author

I'll get back on this as soon as I am back from holiday. Still the intl files are not necessary because they will be generated by a build step. After that step it will indeed be included in the package.

@martijn00
Copy link
Contributor Author

@deandreamatias Can you run the workflow for CI again?

@martijn00 martijn00 requested a review from deandreamatias May 21, 2024 11:27
@martijn00 martijn00 mentioned this pull request May 21, 2024
5 tasks
@bkozik
Copy link

bkozik commented May 21, 2024

Hiya @martijn00 @deandreamatias. I'm having some trouble trying to use this as we wait for it to be merged. Specifically, nullsafety issues having to do with you removing the ! from the nullable string passed to RegExp.hasMatch in a few places. hasMatch requires a String not a String?, and the shift method still returns T? nullable, the user var is still String?.

I'm not sure how it's building on your ends?

Any help would be appreciated, let me know if you need more information or if I'm missing something! Hopefully the below screenshots help

The error:
image
The problem:
image
Nothing else changed to make this non-nullable:
image

image image

https://api.flutter.dev/flutter/dart-core/RegExp/hasMatch.html

@martijn00
Copy link
Contributor Author

@bkozik Good catch, i've added a fix for this now. Can you test it?

@martijn00 martijn00 requested a review from deandreamatias May 22, 2024 08:11
@martijn00
Copy link
Contributor Author

@deandreamatias is it possible that you enable a setting to build updates to a PR in CI automatically without approval every time?

@deandreamatias
Copy link
Contributor

I think that need update the readme section about contribute with languages https://github.com/flutter-form-builder-ecosystem/form_builder_validators?tab=readme-ov-file#add-new-supported-language

@deandreamatias
Copy link
Contributor

About the last error, maybe is because this line flutter build ios --debug --no-codesign. The debug is not used anymore by Flutter? I do not know

@martijn00
Copy link
Contributor Author

martijn00 commented May 22, 2024

@deandreamatias Aah this is because i moved to building on Linux. Do you think i should move it back to MacOS or is it fine for you to only test if Android builds?

I've committed a chance to only do Android, let me know if that works for you, or i will revert back to Mac.

@martijn00
Copy link
Contributor Author

@deandreamatias This seems good to go now. Can you merge and make the release?

@deandreamatias deandreamatias merged commit 53209a9 into flutter-form-builder-ecosystem:main May 22, 2024
3 checks passed
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