Skip to content

Conversation

@mikeprosserni
Copy link
Contributor

@mikeprosserni mikeprosserni commented Sep 2, 2025

  • This contribution adheres to CONTRIBUTING.md.
  • I've updated CHANGELOG.md if applicable.
  • I've added tests applicable for this pull request

What does this Pull Request accomplish?

stream_readers.py and stream_writers.py were very large files, which made development and review more difficult. This PR splits the classes in them out into individual files to make things more manageable.

I was careful to keep each class unchanged through this refactor. I copy-pasted each entire class and made no edits within the classes. The only changes would be in the imports sections.

Why should this Pull Request be merged?

This should make reviewing future changes easier.

What testing has been done?

  • All existing tests pass
  • lint and mypy report no errors
  • I checked the generated sphinx documentation for any changes. The files are largely unchanged, with only a few minor modifications:
    • An extra top level blurb: "NI-DAQmx stream writers. This package provides classes for writing samples to NI-DAQmx tasks."
    • Source code links changed from (for example) stream_writers.html to stream_writers/analog_multi_channel_writer.html
    • <nidaqmx.stream_writers.UnsetAutoStartSentinel object> changed to <nidaqmx.stream_writers._channel_writer_base.UnsetAutoStartSentinel object>
image image

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

Test Results

    38 files  ±0      38 suites  ±0   1h 1m 55s ⏱️ +21s
 2 675 tests ±0   2 232 ✅ ±0    443 💤 ±0  0 ❌ ±0 
47 382 runs  ±0  40 226 ✅ ±0  7 156 💤 ±0  0 ❌ ±0 

Results for commit 00f4176. ± Comparison against base commit 859c18d.

♻️ This comment has been updated with latest results.

Mike Prosser added 2 commits September 2, 2025 11:09
@zhindes
Copy link
Collaborator

zhindes commented Sep 2, 2025

Just snooping, but... love this. Thank you!

@mikeprosserni mikeprosserni changed the title [Under Construction] Refactor stream_readers and stream_writers into smaller files Refactor stream_readers and stream_writers into smaller files Sep 2, 2025
Mike Prosser added 2 commits September 2, 2025 12:03
@mikeprosserni mikeprosserni marked this pull request as ready for review September 2, 2025 17:06
@mikeprosserni mikeprosserni merged commit f22ad8b into master Sep 3, 2025
27 checks passed
@mikeprosserni mikeprosserni deleted the users/mprosser/stream-file-cleanup branch September 3, 2025 17:51
Copy link
Collaborator

@bkeryan bkeryan Sep 5, 2025

Choose a reason for hiding this comment

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

<nidaqmx.stream_writers.UnsetAutoStartSentinel object> changed to <nidaqmx.stream_writers._channel_writer_base.UnsetAutoStartSentinel object>

This sounds like compatibility breakage. Previously, clients could import UnsetAutoStartSentinel and AUTO_START_UNSET, and now they can't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fixing this in #825

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that UnsetAutoStartSentinel is deleted right after being declared, so users could never import it, they could only import AUTO_START_UNSET itself.

class UnsetAutoStartSentinel:
    pass


AUTO_START_UNSET = UnsetAutoStartSentinel()

del UnsetAutoStartSentinel

Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR introduces public submodules, which introduces another way for clients to import these classes. Shouldn't the new submodules be private?

Most of the pain this causes is related to documentation, but it sounds like autodoc is handling this better than autoapi would.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We used private submodules for task, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fixing this in #825

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.

4 participants