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

Feed and Aggregator docs #137

Merged
merged 4 commits into from
Jun 25, 2020
Merged

Feed and Aggregator docs #137

merged 4 commits into from
Jun 25, 2020

Conversation

jlashner
Copy link
Collaborator

@jlashner jlashner commented Jun 2, 2020

This PR updates the Feed and Aggregator docs to be up to date with the current version of ocs. Let me know if you have any suggestions!

Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

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

This is a great improvement to the documentation, thanks!

Please re-fill your paragraphs... we want this to read reasonably well in raw text.

In the dev guide table of contents, I think feeds.rst should be after agents.rst.

Currently register_feeds is shown in the Agent example (agents.rst). It would be really nice to link to this section for more details.

docs/developer/feeds.rst Outdated Show resolved Hide resolved
docs/developer/feeds.rst Outdated Show resolved Hide resolved
docs/developer/feeds.rst Outdated Show resolved Hide resolved
self.log.info("Subscribed to feed {}".format(feed_address))
* - fresh_time
- Time (seconds) before feed is considered "stale", and is removed from
the HK status frame
Copy link
Member

Choose a reason for hiding this comment

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

Where are the agg_params actually decoded? What are the defaults? I assume this is in aggregator agent somewhere, and if so it would be good to link to it.

Also, this is now a superset of what is described in register_agent docstring. So it's possible we've got this information in 3 different places now...

Copy link
Member

Choose a reason for hiding this comment

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

This is an argument to the Provider, so they're described in the Provider docstring.

In the example above, the keys of the ``G3TimesampleMap`` will be
``field_name_1`` and ``field_name_2``.
The ``block_name`` is only used internally and will not be written
to disk.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add... "So field names must be unique within a single feed, and should be descriptive when block_name is not known" or similar?

docs/developer/feeds.rst Show resolved Hide resolved
@mhasself
Copy link
Member

mhasself commented Jun 4, 2020

Whoops... I didn't review aggregator.rst changes... will add that as a second review in a moment.

@mhasself
Copy link
Member

mhasself commented Jun 4, 2020

Changes to aggregator.rst look fine.

I've raised #138 because I think there are some other issues unrelated to the present PR.

@BrianJKoopman BrianJKoopman linked an issue Jun 16, 2020 that may be closed by this pull request
@BrianJKoopman BrianJKoopman linked an issue Jun 16, 2020 that may be closed by this pull request
Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

Couple of minor comments below.

docs/agents/aggregator.rst Outdated Show resolved Hide resolved
docs/agents/aggregator.rst Outdated Show resolved Hide resolved
docs/agents/aggregator.rst Outdated Show resolved Hide resolved
docs/agents/aggregator.rst Show resolved Hide resolved
docs/developer/feeds.rst Show resolved Hide resolved
docs/developer/feeds.rst Show resolved Hide resolved
@simonsobs simonsobs deleted a comment from simonscryo Jun 24, 2020
@jlashner
Copy link
Collaborator Author

Alright! Implemented suggestions. It would be awesome if we could get this merged before the hack session tomorrow, so if there are any minor comments that you don't think should stop the merge feel free to create an issue or add them to Matthew's existing issue.

docs/agents/aggregator.rst Outdated Show resolved Hide resolved
docs/agents/aggregator.rst Outdated Show resolved Hide resolved
docs/developer/feeds.rst Outdated Show resolved Hide resolved
docs/developer/feeds.rst Outdated Show resolved Hide resolved
docs/developer/feeds.rst Outdated Show resolved Hide resolved
self.log.info("Subscribed to feed {}".format(feed_address))
* - fresh_time
- Time (seconds) before feed is considered "stale", and is removed from
the HK status frame
Copy link
Member

Choose a reason for hiding this comment

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

This is an argument to the Provider, so they're described in the Provider docstring.

Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

Awesome, thanks. I caught a couple of types. All the comments are pretty direct and simple suggestions/fixes (sorry to add a couple of new ones, but they're small.) Feel free to fix and merge.

@BrianJKoopman
Copy link
Member

Latest round of comments addressed, will merge when tests are done.

@BrianJKoopman BrianJKoopman merged commit 790443c into master Jun 25, 2020
@BrianJKoopman BrianJKoopman deleted the feed-aggregator-docs branch June 25, 2020 18:50
@BrianJKoopman BrianJKoopman added this to the v0.7.0 milestone Jul 20, 2020
@BrianJKoopman BrianJKoopman mentioned this pull request Jul 31, 2020
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.

Feed documentation is out of date
3 participants