Skip to content

Improvements to existing NSDataLink code #176

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

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.

@gcasa gcasa requested a review from rfm June 21, 2025 19:17
@gcasa gcasa marked this pull request as ready for review June 21, 2025 19:18
@gcasa gcasa requested a review from fredkiefer as a code owner June 21, 2025 19:18
@gcasa
Copy link
Member Author

gcasa commented Jun 21, 2025

The test for this is here

https://github.com/gcasa/NSDataLink_test

I will incorporate this into the test suite after some refinement.

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.

2 participants