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

refactor: Add more native type hints #626

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

DannyvdSluijs
Copy link
Contributor

This PR follows up on #625 and adds more type hints into the library. I did a couple of test runs in order to validate all works as it should.

My current focus is to first add type hints to all the non generated bits and bobs of code and at some point update the generating code to update all the models with the types, such as adding the hints for:

  • protected array $fillable = [ ... ]
  • protected string $url
  • protected string $primaryKey

Copy link
Collaborator

@remkobrenters remkobrenters left a comment

Choose a reason for hiding this comment

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

This is going to be a lot of fun. Nice changes. Like it.

@DannyvdSluijs
Copy link
Contributor Author

@remkobrenters I was curious is there anything you want me to do with this PR or is it worth merging this into main?

@remkobrenters
Copy link
Collaborator

Sorry for my open end on this one.

As I understood (maybe incorrectly) this is the first of a set of changes regarding improvement of types right? As maybe we want to group a couple of these MR's into a branch that results in a bigger bump of the project on this topic (allows also some room for like breaking changes and a major version upgrade if needed). Or is this overkill and would you like to just perform them as smaller updates over the time?

@DannyvdSluijs
Copy link
Contributor Author

I would lean towards the option of smaller improvement as separate PR's, as I can think of two downsides of a single large set of changes:

  1. Increased "risk": It becomes more difficult to review and could result in a "big-bang merge" with increased risk in bug amount. Hence my slight preference to do small incremental improvements over time.
    Kind of like in the following tweet although value in this case isn't measured in dollars but in developer experience.
  2. ease of commitment: I'm more than happy to help but a larger set requires more commitment over a shorter time span (in order to keep track of progress) compared to small improvements which can be short chunks of time spread over a longer periode of time.

I also recognise both you and @stephangroen as the maintainers and therefore can make a decision either way.

@remkobrenters
Copy link
Collaborator

Too much credits for me here :) I am just a preparation step to make life a bit easier for @stephangroen.
I understand the reasoning and for me it is perfectly fine to make smaller updates over time so lets go ahead with this one.

@stephangroen
Copy link
Member

Thank you again for all of your efforts @DannyvdSluijs. Both you and @remkobrenters have put quite some time and effort into this library/package. I'm very grateful for your efforts and both of you deserve the credits :)

I'm also more fan of the smaller updates. This keeps it simple and more clear. It's time the code moves toward type hints. Let's continue on this path.

@stephangroen stephangroen merged commit 2c66c28 into picqer:main Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants