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

[pending] Documentation for pyDKB.common.json_utils. #169

Open
wants to merge 11 commits into
base: pyDKB-master-merge
Choose a base branch
from

Conversation

Evildoor
Copy link
Contributor

@Evildoor Evildoor commented Sep 6, 2018

Basic documentation for pyDKB.common.json_utils.


Waits for #165

@mgolosova mgolosova changed the title Added documentation for pyDKB.common.json_utils. [pending] Added documentation for pyDKB.common.json_utils. Sep 6, 2018
@mgolosova
Copy link
Collaborator

After (or with) every change in docs I would add new version of HTML and PDF documentation; after all, it is the destination of any changes in docstrings ;)

Utils to work with JSON (dict) objects.
Utils to work with JSON objects.

In context of python, JSON [#]_ objects may be considered as structures
Copy link
Collaborator

Choose a reason for hiding this comment

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

Python?

Copy link
Contributor Author

@Evildoor Evildoor Sep 7, 2018

Choose a reason for hiding this comment

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

According to my knowledge, JSON is not tied to python in any way - python is just another language which can work with this format. Other languages may not even have things such as 'dictionary' and use different terms to define the same object. Therefore, I believe that saying "you see, JSON file is usually just a big dictionary which may contain other dictionaries, lists, ..." is wrong without indicating that you are talking in terms of python (even considering the fact that this is mostly python project).
Side note - thanks, due to this comment I've found that I forgot to mention some other possible objects such as True/False.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, bold symbols are not that bold when you don`t know they are here...
I meant "shouldn`t Python be capitalized?"

Copy link
Contributor Author

@Evildoor Evildoor Sep 7, 2018

Choose a reason for hiding this comment

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

Ha, so that's what this was about! Sure, will do.

STRING key -- dot-separated list of nested keys.
If a key contains dot itself, the key must be put between
quotation marks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I`d suggest to follow common style in docstrings.

I started to use this notation:

def func(arg1, agr2):
   """ Do this and that.

    Detailed description, warnings, notes and ToDo.

    :param arg1: first argument meaning
    :type arg1: str
    :param arg2: second argument meaning
    :type arg2: int, float, NoneType

    :return: this or that (None if failed)
    :rtype: dict, NoneType
    """

It produces pretty predictable view after building (please see e.g. pyDKB.common.custom_readline() in the compiled documentation -- it is not perfect, yet uses stuff I`m talking about).
If it is OK then let`s follow this style; if not, let`s find (or invent) another and use it everywhere.

Google also says that there`s a keyword :raises, yet I never tried it yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I am going to add a “howto” to the repo, so that we had a common guide for this work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, I'll take a look at custom_readline().

**Examples:**
Transform STRING ``\'1.2.3'`` into LIST ``[\'1', \'2', \'3']``.

Transform STRING ``\'1.\"2.3".4'`` into
Copy link
Collaborator

Choose a reason for hiding this comment

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

Talking about examples — I would suggest example in form of Python code, something like:

>>> nestedKeys('1.2.3')
['1', '2', '3']

Having less words it looks more illustrative for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. However, per our discussion yesterday - tests will be used as examples, so I'll remove examples I've made here.

STRING key -- dot-separated list of nested keys.
If a key contains dot itself, the key must be put between
quotation marks.
If a key contains dot itself, the key must be put
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not forget that originally it was written with no concern to Sphinx, reStructuredText and building documentation from this comment. Thus newline in the comment does not mean newline or new paragraph in the built version — check how it looks now. Is it what you expected to get there?

Copy link
Contributor Author

@Evildoor Evildoor Sep 7, 2018

Choose a reason for hiding this comment

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

It means a newline in built version when [source] button is clicked.
image
Normal version is unaffected.

Copy link
Collaborator

@mgolosova mgolosova Sep 7, 2018

Choose a reason for hiding this comment

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

Sorry, didn`t say that I was talking about PDF version.
HTML looks fine -- couldn`t check it from tablet ;)

"""


def valueByKey(json_data, key):
""" Return value by a chain (list) of nested keys.

Parameters:
It is common for JSON objects to contain many layers of dictionaries
nested in other dictionaries - this function extracts the data from
Copy link
Collaborator

Choose a reason for hiding this comment

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

Single hyphen produces short dash, double — long one. In this case I guess you meant to use the long one, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, thanks. Will fix.

@Evildoor
Copy link
Contributor Author

Updated the docs according to comments.

@mgolosova, due to software reasons known to you I cannot make PDF version at the moment. Should I make and upload HTML version or leave this issue to you?

@mgolosova
Copy link
Collaborator

@Evildoor
I`m working on the issue you`re talking about, sorry it takes a bit long...
I think I will build the docs for this PR if fail to solve the issue soon enough.
Thank you for the changes — I`ll get through them carefully tonight, chacking how the built version looks.

Yet I have a couple of comment already.
It`s better not to refer in the commit logs to what can not be found in the log history. After all, one may be reading it in console ;)
Also, “minor fixes” commit contains three different changes. They are minor, right, but still logically independent. If you fill like joining commits — better to join with the related ones, e.g. with the first one, where the documentation has largely changed.
And, talking about commit organization — the very first one states “addaed documentation for...json_utils”, while what we see in the commit log is:

  • extended module description;
  • added detailed description of valueByKey;
  • changed formatting of parameters description;
  • added examples.

It is a bit too much for such a laconic log message ;) I see two ways:

  • make one commit “Update documentation (<module_name>)” (as you have done), but list changes in the log;
  • make 4 commits for the 4 mentioned changes.

I do remember that the last change (examples) was later reverted, so it is up to you — leave it and revert or simply remove — but the general idea is kind of this.

@mgolosova
Copy link
Collaborator

mgolosova commented Sep 13, 2018

@Evildoor
I believe I finally managed to configure TeX on the bamboo server. Please check, and if there are any problems (except that pdflatex claims about fancyvrb.cfg) please let me know.

- Extended module description
- Extended valueByKey() description
- Reformatted the "parameters" sections
- Added examples
It was decided that tests should be used as examples.
The following changes were also made to match the new style:
- Rephrased "key" description in both functions because saying
"list of keys" about a variable which is explicitly defined
as "str" in the next line is confusing.
- Removed type highlights from nestedKeys() description.
@Evildoor
Copy link
Contributor Author

@mgolosova
Thank you, the PDF builds now. Also made the changes according to your comments.

Copy link
Collaborator

@mgolosova mgolosova left a comment

Choose a reason for hiding this comment

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

Now almost everything looks fine, thank you!

@@ -57,6 +57,8 @@ def nestedKeys(key):

String should contain keys separated by dot. If a key contains
dot itself, the key must be put between matching quotation marks.
Quotation marks inside the keys (not preceding or following a dot)
are ignored.
Copy link
Collaborator

Choose a reason for hiding this comment

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

By "ignored" you mean "treated as ordinary symbols", right?
If I read the description without knowing how the function works (or supposed to), for a moment I might think that "ignored" means "removed from the output", like:

>>> nested_keys('ab.c"d.e')
['ab', 'cd', 'e']

are ignored.

:param key: nested keys
:type key: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

:type key: str, list

If passed key is of type list, it is returned without changes.

:param json_data: to search in
:type json_data: dict
:param key: nested keys
:type key: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

:type key: str, list

If passed key is of type list, it is treated as "already parsed" nested keys.

Nested keys can be list instead of str in both functions - it is
treated as already processed str and used as-is.
@Evildoor
Copy link
Contributor Author

Done.

@mgolosova mgolosova added the Docs label Sep 18, 2018
STRING key -- dot-separated list of nested keys
It is common for JSON objects to contain many layers of dictionaries
nested in other dictionaries -- this function extracts the data from
such constructions according to given string or list with keys.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The description does not explain how keys should be passed. If usage of list version can be guessed, string is not clear at all.
What about something like this?

...according to a list of keys. 
<...>
:param keys: sequence of nested keys (dot-separated string or list object)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will do.

@mgolosova
Copy link
Collaborator

Everything seems to be fine; now waiting for the pyDKB fix...

@Evildoor Evildoor changed the title [pending] Added documentation for pyDKB.common.json_utils. [pending] Documentation for pyDKB.common.json_utils. Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants