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

Added Support for Duration Objects. #417

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Speiger
Copy link

@Speiger Speiger commented Nov 3, 2024

Simple support for Duration Objects.

Since they are cleaner to work with than milliseconds directly or using weird number * timeunit multiplications.
Not saying they are bad, but Duration objects are just cleaner to work with due to their helper functions.

I included tests and ran them with no errors.
Though the drawTextWithSpecialCharacters tests fails on both PRs i made so far.

These are simply cleaner to work with. Yes you can convert them to
millseconds but honestly if there is already a timeunit option a
duration option should exist too.
@Speiger Speiger changed the base branch from master to develop November 3, 2024 12:18
@Speiger
Copy link
Author

Speiger commented Nov 5, 2024

Added tests for them to assure the logic works the exact same.
Ran the tests locally and they work just fine.

From my side the PR is finished.

@kokorin
Copy link
Owner

kokorin commented Nov 5, 2024

From my side the PR is finished.

Please, pay attention to code style accepted in the project.

@Speiger
Copy link
Author

Speiger commented Nov 5, 2024

@kokorin yep already did.
If you need something extremely specific then please name them all in order :)

@Speiger
Copy link
Author

Speiger commented Nov 5, 2024

@kokorin
Let's make this simpler.

I think the code quality filter you have setup is extremely picky.
(Especially in the documentation like WTF)
Too picky IMO.

I went out of my way to make these patches and provided them in a reasonable state.

Anything extra is for you to uphold.
In other words please to the final polishing cut yourself.

If you don't want to do that and think that mentality is unreasonable, then please close both prs.
Because that just wastes time for everyone involved.

@Speiger
Copy link
Author

Speiger commented Nov 9, 2024

@kokorin please provide a choice.

@kokorin
Copy link
Owner

kokorin commented Nov 9, 2024

From my side the PR is finished.

Please, pay attention to code style accepted in the project.

Already did

@Speiger Speiger closed this Nov 9, 2024
@Speiger
Copy link
Author

Speiger commented Nov 9, 2024

Yeah, and at this point you lost me.
I rather deal with a self maintained fork then with these, IMO excessive, documentation requirements.
If you want to be picky that's fine, but also expect people not to 100% agree on that and ask you to meet halfway.
I mean you had enough time to review the PR too.

Anyways, I work with my fork instead of helping.

Ignoring this issue.
Have a nice weekend!

@kokorin kokorin reopened this Nov 9, 2024
@Speiger
Copy link
Author

Speiger commented Nov 9, 2024

Small fun fact, @kokorin Eclipse, my choice of IDE doesn't support Editor configs so in other words: I can't apply your style configs at all.

In other words unless I am switching the IDE, which i am not willing to, that style check won't be completed.

So yeah, the question comes again:
I would like to see a response from you based on if you want to finish it or not.

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