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

Improvements to existing NSDataLink code #176

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

gcasa
Copy link
Member

@gcasa gcasa commented Mar 17, 2023

These changes will implement the rest of NSDataLink/NSDataLinkManager. This class was not often used, but is relatively easy to implement. This class was rarely used, but should be easy to finish.

@rfm
Copy link
Contributor

rfm commented Dec 22, 2023

Looking at the diffs, it seems to me that the changes are almost all cosmetic (improving ivar names and changing whitespace).
I think it would make things far easier to review if there were separate changes:

  1. a small set of functional changes to be reviewed carefully for correct logic
  2. cosmetic changes, which can be passed very quickly

I generally try to do cosmetic stuff separately first.

@gcasa
Copy link
Member Author

gcasa commented Dec 22, 2023

Looking at the diffs, it seems to me that the changes are almost all cosmetic (improving ivar names and changing whitespace). I think it would make things far easier to review if there were separate changes:

  1. a small set of functional changes to be reviewed carefully for correct logic
  2. cosmetic changes, which can be passed very quickly

I generally try to do cosmetic stuff separately first.

They are at this point. My original intention with this PR was to make the class actually work. I implemented what is there a long time ago, but got stuck on how to monitor changes to a file without polling it (which is terribly inefficient) so, I wanted to come back to it and see if I could find a way to make it functional. Sometimes I do the "cosmetic" stuff while I am thinking as it helps me think more cleanly. I don't know if that makes sense, but it's my process. ;)

Point taken. It would be understandable from the reviewer's point of view. When I did this I hadn't considered that.

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.

None yet

2 participants