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

Reorganise API docs #1478

Merged
merged 28 commits into from
Mar 7, 2019
Merged

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Feb 15, 2019

This is by no means complete but is a start towards a more sensible organization of the api docs

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

some minor comments.

qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
docs/api/index.rst Outdated Show resolved Hide resolved
docs/api/dataset.rst Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
@jenshnielsen jenshnielsen force-pushed the docs/reorg_api branch 2 times, most recently from cb63625 to 154e44f Compare February 22, 2019 13:36
Mocking dependencies does not seem to work correctly for the summary

The slack module is not important anyway
@jenshnielsen jenshnielsen marked this pull request as ready for review March 7, 2019 13:35
@jenshnielsen jenshnielsen changed the title [WIP] Reorganise API docs Reorganise API docs Mar 7, 2019
@jenshnielsen
Copy link
Collaborator Author

I think this is ready now.

  • There is a .rst file per python file (module) that we document
  • These are kept in a folder structure that matches the code
  • We only document the relevant modules. (Apart from utils etc where we document everything)
  • The individual python files are documented with a automodule clause. This means that everything is
    included for a module. We could get better results by going more fine grained and chose the specific classes to document but I think this is the best trade off between maintainability and granularity)

@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #1478 into master will decrease coverage by 0.02%.
The diff coverage is 50%.

@@            Coverage Diff            @@
##           master   #1478      +/-   ##
=========================================
- Coverage   73.82%   73.8%   -0.03%     
=========================================
  Files          92      92              
  Lines       10438   10444       +6     
=========================================
+ Hits         7706    7708       +2     
- Misses       2732    2736       +4

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

I'm waiting for the CI to finish, and then i'll look through the built docs....

docs/api/dataset/index.rst Outdated Show resolved Hide resolved
docs/api/instrument/index.rst Outdated Show resolved Hide resolved
metadata
plotting
slack
threading
Copy link
Contributor

Choose a reason for hiding this comment

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

so what are the steps for the developer once he adds a new python module that deserves being included in the docs? perhaps, those can be added to "contributing" or wherever?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that would be great once that is rewritten.

Copy link
Contributor

Choose a reason for hiding this comment

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

can a rudimentary description be a part of this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but the problem is not the description. The problem is that it would land in the middle of another document where the rest no longer applies

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd be ok with that because without the description i'd have to bother you when i make "my next PR" :) Full-blown proper fixing of that document can be done later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

good! thanks!

@astafan8
Copy link
Contributor

astafan8 commented Mar 7, 2019

Is it possible and is it preferred to go from

qcodes.dataset.database_extract_runs.extract_runs_into_db(source_db_path: str, target_db_path: str, *run_ids, upgrade_source_db: bool = False, upgrade_target_db: bool = False) → None

to

extract_runs_into_db(source_db_path: str, target_db_path: str, *run_ids, upgrade_source_db: bool = False, upgrade_target_db: bool = False) → None

when generating docs?

@jenshnielsen
Copy link
Collaborator Author

Is it possible and is it preferred to go from

qcodes.dataset.database_extract_runs.extract_runs_into_db(source_db_path: str, target_db_path: str, *run_ids, upgrade_source_db: bool = False, upgrade_target_db: bool = False) → None

to

extract_runs_into_db(source_db_path: str, target_db_path: str, *run_ids, upgrade_source_db: bool = False, upgrade_target_db: bool = False) → None

when generating docs?

I would mostly be against that. Explicit is better than implicit and all that

@astafan8
Copy link
Contributor

astafan8 commented Mar 7, 2019

I'm just going to throw them here since i've found them, but it's up to the author to decide what to do with them.

Have problems with formatting:

  • class qcodes.utils.validators.Validator
  • qcodes.utils.plotting.Number has a docstring which does not correspond to it
  • qcodes.utils.plotting.auto_range_iqr - interesting "return type" entry, i don't know how it should be actually, maybe it's ok and just confusing for me
  • qcodes.utils.helpers.NumpyJSONEncoder - list of supported conversions in "default" method is badly formatted. Also, the docstring includes the base class docstring which i think is irrelevant here.
  • station.remove_component has an extra bullet point for "raises" section
  • field_vector, set_component method: extra bullet point in the "parameters" section
  • qcodes.logger.log_analysis - some of the "returns" sections are not formatted correctly (they look like quotes instead)
  • qcodes.logger.instrument_logger.InstrumentLoggerAdapter docstring can be improved to have code/examples/plain text distinguished
  • qcodes.instrument.group_parameter.GroupParameter - it's a bit bad than the doc is polluted with the methods of the parent class
  • qcodes.instrument.parameter module has a nice module-level doc but cool stull like ParameterWithSetpoints is not mentioned there while ArrayParameter is
  • qcodes.instrument.channel.AutoLoadableChannelList - the bullet-point list in the docstring is not renderred as such

@jenshnielsen
Copy link
Collaborator Author

@astafan8 I was going to suggest that we should split up all the modules and each do a pr per module to bring the docs up to spec for that module.

  • That would be fix the issues that you have seen
  • Add missing module level docstrings
  • Add other missing docstrings

I really just wanted this to be about a sane overall layout

@astafan8
Copy link
Contributor

astafan8 commented Mar 7, 2019

we should split up all the modules and each do a pr per module to bring the docs up to spec for that module

Perfect! Let's do that soon.

@jenshnielsen jenshnielsen merged commit e0fdefb into microsoft:master Mar 7, 2019
@jenshnielsen jenshnielsen deleted the docs/reorg_api branch March 7, 2019 15:24
@jenshnielsen
Copy link
Collaborator Author

I will do the contributing guild updates in a new pr now

giulioungaretti pushed a commit that referenced this pull request Mar 7, 2019
Merge: ffa889b f4638a1
Author: Jens Hedegaard Nielsen <[email protected]>

    Merge pull request #1478 from jenshnielsen/docs/reorg_api
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.

2 participants