pull request

[oldgithubman]

No description provided.

[eoyilmaz]

Hey @oldgithubman thank you for the PR, you did a lot of changes. It would be lovely if you can update your PR comment with some insight on the PR. Did you fix any of the bugs, did you introduce any new features etc.

I pushed some changes back, as you code was not working for MacOS (and possibly for Linux).

This PR needs more work... and I'll be keep updating it tonight.

Many thanks.

[oldgithubman]

Hey @oldgithubman thank you for the PR, you did a lot of changes. It would be lovely if you can update your PR comment with some insight on the PR. Did you fix any of the bugs, did you introduce any new features etc.

I pushed some changes back, as you code was not working for MacOS (and possibly for Linux).

This PR needs more work... and I'll be keep updating it tonight.

Many thanks.

You're welcome. I'm still working on it too, but I'm a beginner and don't really know what I'm doing, just FYI, so please review carefully. My OCD makes writing incredibly difficult to impossible. I suppose you can consider this a "rolling PR" (I might have just made that up). I'm just fixing things as I see them. I don't know if I've fixed any issues on the tracker, but I've fixed countless bugs. I haven't introduced any new features, perse, unless you count the windows tests.

I haven't managed to get all the tests to pass yet. It's very much a work-in-progress. Once I'm done working on agw, my plan is to resume just trying to get the tests to pass. I'm developing for and on windows and vscode just greys out all the macos and linux code and doesn't provide any insight, so I'm kinda in the dark for those OS's. I'm hoping you can help on that front.

Again, this PR should be considered "rolling." It's really just a reflection of the current state of my fork. Are you able to pick things out as I go? Take what's helpful and tell me what's not so I can make appropriate changes and remain in sync with your repo?

Also again, I don't really know what I'm doing (with software dev or git/github), so I'm open to learning better ways of doing things, as long as they're compatible with my OCD.

Thanks for all your work. I plan on making significant contributions =;-)

[eoyilmaz]

Hmm... If you are still working, I might have overwritten on your commits (just force pushed new commits), be careful.

Otherwise, thank you, I liked that you are refactoring the code and splitting the unnecessarily long functions into smaller ones, this is definitelly the way we need to go. I'm still going over your code and modifying it as I see fit.

[eoyilmaz]

Ah, it would be good if you rebase your own changes instead of merging.

[oldgithubman]

Hmm... If you are still working, I might have overwritten on your commits (just force pushed new commits), be careful.

Otherwise, thank you, I liked that you are refactoring the code and splitting the unnecessarily long functions into smaller ones, this is definitelly the way we need to go. I'm still going over your code and modifying it as I see fit.

Yeah, I'm still working. I resolved a bunch of conflicts by accepting your changes. Is this what you're talking about?

Are you married to having the triple quotes inlined with the docstrings? It deprives us of three characters, which does make a difference sometimes. I'm ok with whatever you decide. Also, regarding 88 characters, I read somewhere (I think the flake8 documentation) that the intention is to still aim for 80 with the extra eight being slack. If you reference my .flake8 file, I've modified it to be aligned with their recommendations. Again, whatever you decide is fine, just wanted to make sure you're aware of the intent behind 80/88 and the .flake8 recommended setup

edit - https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html
I think that's where i got my understanding. check out the section on flake8 and bugbear. I might have misinterpreted it

[oldgithubman]

Ah, it would be good if you rebase your own changes instead of merging.

I'l try to do that next time. This is all new to me. Thanks for the advice

[oldgithubman]

Do you use element/matrix? It might be nice to have some realtime coms

[eoyilmaz]

Do you use element/matrix? It might be nice to have some realtime coms

Ah, no, I'm not using it.

[oldgithubman]

Do you use element/matrix? It might be nice to have some realtime coms

Ah, no, I'm not using it.

ok. then just be sure to tag me if it's urgent

[oldgithubman]

That's all the work I've already done. Resuming work. I'll switch to your format going forward unless you tell me otherwise (""" inline with first sentence of docstrings and 88 characters)

[oldgithubman]

I tried to rebase it this time. I don't know if I did it right. Then I was still a couple of new commits behind, so I clicked "update branch" on the github site and it looks like it merged again? Guess I'll stick to CLI/vscode. What do you guys use?

BTW, my flake8 config is effectively 88 characters, even though it says 80, so don't be afraid to merge it

Edit - FYI, I'm moving the flake8 config to setup.cfg to consolidate

[oldgithubman]

ok, I obviously still don't have this rebase thing figured out. Can anyone help?

[p5k369]

Hey @oldgithubman,
if you are not familar with git, then this might be a good starting point for you, it visualizes git behaviour and teaches you everything (and a little more) you need to know for handling branches: https://learngitbranching.js.org/?locale=en_US

Besides that, you merge (in general) when you created a feature branch, developed something and then want to bring it back to your base branch.
For example you decide to create a branch from eoyilmaz develop branch, name it 'windows github test runners implemented' write code that does exactly that, push your changes to your branch and then create a pull request to the base branch of eoyilmaz. That way your pull request can be reviewed and easily merged if accepted.

You want to rebase (in general) if you created a feature branch, developed something and in the meantime another developer changed the base branch. You do not want to rebase already pushed changes.
https://www.atlassian.com/git/tutorials/merging-vs-rebasing#the-golden-rule-of-rebasing

Keep up the good work, I think you would benefit from more and smaller pull requests.

[oldgithubman]

Hey @oldgithubman, if you are not familar with git, then this might be a good starting point for you, it visualizes git behaviour and teaches you everything (and a little more) you need to know for handling branches: https://learngitbranching.js.org/?locale=en_US

Thank you. I'll check that out. I think I might have figured it out though. I do a vscode branch rebase on upstream and then force push. Does that sound right? It seems to have worked, although it looks messy here.

Keep up the good work, I think you would benefit from more and smaller pull requests.

Thanks again. I think DisplayCAL would benefit from more and smaller pull requests from me. Unfortunately, at this stage in my work, that's just incompatible with my ADHD/OCD. It's hard to explain. I start working on one thing and then I notice something else that needs fixing, and I sort of just "spiral out," fixing things as I encounter them. I tried to explain my concept of a "rolling PR" in a previous comment. To clarify, the intent of this PR isn't to be pulled wholesale with the expectation of everything working (yet). Originally, I was going to get everything into a working state before I did a PR, but then you guys started making a bunch of commits again, so I wanted to make sure you guys knew what I was working on so we weren't duplicating efforts. I was hoping maybe you guys could pull individual commits you find useful or something (or at least just keep you guys aware). I understand it's not ideal, but it's the best I can do ATM. I hope it's somehow useful.

Is there a better way for me to do this, accounting for my pathological workflow?

[fire332]

I start working on one thing and then I notice something else that needs fixing, and I sort of just "spiral out," fixing things as I encounter them.

Is there a better way for me to do this, accounting for my pathological workflow?

When you need to fix something else, you can either create a new branch locally or stash your commits, keeping in mind to split up your fixes into separate PRs/branches. If you've already added a fix to your main branch, you can use the cherry pick command to split only those changes in your fix to a separate branch and create a PR from there.

It also helps to use a tool/VSCode extension (e.g. GitLens) that lets you visualize the local/remote branches and exactly what a git command will do.

https://www.git-tower.com/learn/git/faq/cherry-pick
https://www.freecodecamp.org/news/git-stash-commands/

[oldgithubman]

ok, once I'm done with what I'm working on (agw, then passing current tests), if we can get this PR merged, I'll try your suggestions

[oldgithubman]

Normally we would work on one feature/bug fix at a time on a branch, And it will be much easier to go through for me and merge it.

Yeah, I've accepted I'm going to have to modify my workflow (after this PR - I'm in too deep). Hopefully, I can adapt.

Rebasing is the cleanest way of merging changes. But, no worries, tomorrow I'll start looking into your branch again. Maybe we should merge your changes in stages, file by file maybe.

I'm pretty sure I'm introducing new bugs during these rebases because I don't really know what I'm doing and it seems legitimately difficult. I'd greatly appreciate if we can get this merged so we're on the same page. Merging in stages is exactly what I was thinking. Once merged, I can switch to the new workflow

[eoyilmaz]

Once merged, I can switch to the new workflow

This will take time.

[eoyilmaz]

Hey @oldgithubman, I started working on your PR, there are a lot of changes that I need to go over and I'm introducing more chagenges on top of your work.

So, if you want to keep working, please open a new branch in your repository (with the name develop-extra or whatever you see fit) originating to your develop branch (so the contents are matching before you start adding more changes) and push your further changes to that branch. So that we don't start jumping over each others changes and try to merge/rebase continously.

After I merge your PR, your other branch should be easy to rebase and merge.

[eoyilmaz]

It took me all day long to go over the DisplayCAL.lib.agw.artmanager, although the changes are mostly welcomed, I can say that you are overly type-hinting. You don't need to hint the types for variables that you defined inside a function/method as the type is obvious, this is unecessary:

_RENDER_VER: tuple[int, int, int, int] = (2, 6, 1, 1)

the following is enough as the types are obvious:

_RENDER_VER = (2, 6, 1, 1)

See my changes, you'll understand.

[oldgithubman]

Regarding the # noqa: SC100 comments, I was already in the process of removing them. But you don't have to ignore spell check. Instead, I created a whitelist.

Regarding your refactors, I disagree with many of your changes... I had artmanager.py down to zero problems. You've introduced 95 new problems so far.

Regarding type hints, you've removed useful hints that are now confusing the type checker (some were probably unnecessary, but you've removed useful ones too).

@eoyilmaz before you keep going, maybe we should discuss this

Judging by some of these changes, I'm not sure you're even working with my latest changes

[eoyilmaz]

Regarding the # noqa: SC100 comments, I was already in the process of removing them. But you don't have to ignore spell check. Instead, I created a whitelist.

That's good, thank you. Removed SC100's and adding exceptions to whitelist.txt

Regarding your refactors, I disagree with many of your changes... I had artmanager.py down to zero problems. You've introduced 95 new problems so far.

I don't know how you check it but when I ran flake8 I saw 45 and most of them are for useless things, 1 was because I reverted one of your changes. Fixing the all of them in a minute.

Regarding type hints, you've removed useful hints that are now confusing the type checker (some were probably unnecessary, but you've removed useful ones too).

I don't see any more needed.

Judging by some of these changes, I'm not sure you're even working with my latest changes

I guess, as you did, I started with the original implementation of wx.lib.agw from https://github.com/wxWidgets/Phoenix/blob/master/wx/lib/agw and then compared your changes on top of them. Thus, I removed some of your changes that doesn't make sense or breaking things with other OSes.

[oldgithubman]

I don't know how you check it but when I ran flake8 I saw 45 and most of them are for useless things, 1 was because I reverted one of your changes. Fixing the all of them in a minute.

I check it with my flake8 config (which should be pretty close to yours) and pylance with standard type checking. Like I said, I had all problems solved. Now there are 54 pylance problems with the remainder being flake8. If we're going to say problems are useless, we should reconfigure flake8 accordingly. I want to see zero problems in my work (or should I say, my OCD demands it).

I don't see any more needed.

Many of these new pylance problems are type-related.

I guess, as you did, I started with the original implementation of wx.lib.agw from https://github.com/wxWidgets/Phoenix/blob/master/wx/lib/agw and then compared your changes on top of them. Thus, I removed some of your changes that doesn't make sense or breaking things with other OSes.

Can we discuss the things you think don't make sense before changing them? I've spent a lot of time making these changes and it's very demoralizing seeing them reverted without discussion

[p5k369]

Hey @oldgithubman,
thought I drop a comment maybe it helps you.

I want to see zero problems in my work (or should I say, my OCD demands it).

This only works from your own perspective and sometimes leads to mistakes that you can't even recognize because you would have to know everything. Does that make sense?
I give you an example:
You can use built-in collection types for type annotations. In your IDE your linter will tell you everything is fine - no problems that is because you are on 3.11 or 3.12.
But that also means that DisplayCal now no longer supports python-3.8. That can be fine, maybe it's time to drop support.
Not every decision is just right or wrong.

[eoyilmaz]

I can say that you are overly type-hinting

I'm going to back up on this one a little. When I said this, I was thinking type hinting as in Rust. But rethinking it, Python is not Rust and the whole idea behind the type hinting in Python is to help the typecheckers/IDEs to understand what type it should expect. So, I'll introduce some of your type hints back 👍

[oldgithubman]

This only works from your own perspective and sometimes leads to mistakes that you can't even recognize because you would have to know everything. Does that make sense?

I'm not sure I fully understand, but that's why review exists.

I give you an example: You can use built-in collection types for type annotations. In your IDE your linter will tell you everything is fine - no problems that is because you are on 3.11 or 3.12.

Doesn't the linter take into account our project config (requires-python = ">= 3.8")? It certainly should.

Not every decision is just right or wrong.

I agree

[oldgithubman]

I'm going to back up on this one a little. When I said this, I was thinking type hinting as in Rust. But rethinking it, Python is not Rust and the whole idea behind the type hinting in Python is to help the typecheckers/IDEs to understand what type it should expect. So, I'll introduce some of your type hints back 👍

What editor are you using? If you use vscode with pylance (default) and set type checking to standard (edit - I've since changed this to "strict"), you'll be able to see the same problems I do

Edit - I've decided to create and maintain my own fork of Mr. Höch's codebase. https://github.com/oldgithubman/DisplayCAL
Request pull requests (is that a thing?) if you see anything you want. Feel free to close this pull request unless you want to keep working on it. Good luck!