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

Add proper cross-references in docs. #1101

Closed
wants to merge 5 commits into from

Conversation

drewrisinger
Copy link
Contributor

@drewrisinger drewrisinger commented Jul 10, 2018

ONLY significant change is forcing AsyncioServer to inherit from AbstractBaseClass (ABC) to force its _handle_connection_cr() method to be implemented by subclasses.

A few other minor spelling/grammar fixes, too.

Tested using artiq-doc Conda environment

Signed-off-by: Drew Risinger drisinger+github@gmail.com

A few other minor spelling/grammar fixes, too.
Tested using artiq-doc Conda environment

Signed-off-by: Drew Risinger <drisinger+github@gmail.com>
@jordens jordens self-requested a review July 10, 2018 16:53
Copy link
Member

@jordens jordens left a comment

Choose a reason for hiding this comment

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

Just a few minor nits. Great work! Could you say a few words about your affiliation (I guess UMD) and whether there is more high quality contributions coming?

@@ -32,3 +32,6 @@ __pycache__/
/dataset_db.pyon
/device_db*.py
/test*

# Pycharm/Jetbrains Development
.idea/
Copy link
Member

Choose a reason for hiding this comment

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

Please don't. You want to have those in your personal gitignore and not add everyone's favorite editor's temp files to every project.

the ``archive`` argument to ``False``.
modified, a warning is emitted.

:param archive: Set to ``False`` to turn off archiving datasets obtained
Copy link
Member

Choose a reason for hiding this comment

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

"Set to False to prevent archival together with the run's results. Default is True" is a bit simpler and makes fewer assumptions about the workings of other parts of the codebase.


Most experiments should derive from this class."""
def prepare(self):
"""The default prepare method calls prepare for all children, in the
order of instantiation, if the child has a prepare method."""
"""This default prepare method calls :meth:`~artiq.language.environment.Experiment.prepare`
Copy link
Member

Choose a reason for hiding this comment

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

Try to keep it < 80 chars line length.

order of instantiation, if the child has a prepare method."""
"""This default prepare method calls :meth:`~artiq.language.environment.Experiment.prepare`
for all children, in the order of instantiation, if the child has a
:meth:`~artiq.language.environment.Experiment.prepare` method."""
Copy link
Member

Choose a reason for hiding this comment

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

And put the final """ onto a line by itself if the docstring is longer than one line.

server.

Only methods are supported. Attributes must be accessed by providing and
using "get" and/or "set" methods on the server side.

At object initialization, the connection to the remote server is
automatically attempted. The user must call ``close_rpc`` to
automatically attempted. The user must call :meth:`~artiq.protocols.pc_rpc.Client.close_rpc` to
Copy link
Member

Choose a reason for hiding this comment

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

Line length.

be retrieved using ``get_rpc_id`` and then one can be selected later
using ``select_rpc_target``.
be retrieved using :meth:`~artiq.protocols.pc_rpc.Client.get_rpc_id`
and then one can be selected later using :meth:`~artiq.protocols.pc_rpc.Client.select_rpc_target`.
Copy link
Member

Choose a reason for hiding this comment

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

Line length.

@@ -198,7 +198,7 @@ def __init__(self):

async def connect_rpc(self, host, port, target_name):
"""Connects to the server. This cannot be done in __init__ because
this method is a coroutine. See ``Client`` for a description of the
this method is a coroutine. See :class:`artiq.protocols.pc_rpc.Client` for a description of the
Copy link
Member

Choose a reason for hiding this comment

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

Line length.

@@ -70,11 +72,11 @@ If there are other experiments with higher priority (e.g. a high-priority timed
Otherwise, ``pause()`` returns immediately.
To check whether ``pause()`` would in fact *not* return immediately, use :meth:`artiq.master.scheduler.Scheduler.check_pause`.

The experiment must place the hardware in a safe state and disconnect from the core device (typically, by using ``self.core.comm.close()``) before calling ``pause``.
The experiment must place the hardware in a safe state and disconnect from the core device (typically, by calling ``self.coredevice.comm.close()`` from the kernel, which is equivalent to :meth:`artiq.coredevice.core.Core.close`) before calling ``pause()``.
Copy link
Member

Choose a reason for hiding this comment

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

To fix my repeated confusion why we don't have pause() documented at all, Scheduler.pause() should probably be a stub method in the "root" Scheduler that we can cross-ref and document.

@jordens
Copy link
Member

jordens commented Jul 10, 2018

And I guess you tested those changes and crossref targets by running sphinx locally?

Addresses m-labs#1102, m-labs#1102.
User-interface issue, where users could put both -v and -q in to leave logging level unchanged.
Make -v/-q command line args mutually exclusive
@sbourdeauducq
Copy link
Member

ONLY significant change is forcing AsyncioServer to inherit from AbstractBaseClass (ABC) to force its _handle_connection_cr() method to be implemented by subclasses.

There are several other instances in ARTIQ where a method has to be implemented by subclasses, but we're not using abc there (the method simply raises NotImplementedError), so it becomes a bit inconsistent. Can the documentation result you wanted to achieve be done without abc?

sbourdeauducq added a commit that referenced this pull request Sep 26, 2018
@sbourdeauducq
Copy link
Member

c71e442

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.

None yet

3 participants