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

MNT/DOC: parse valid json db, improve docstring #320

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Jul 24, 2023

Description

  • Slightly modifies the happi update command to handle json formatted similar to a valid json database.
  • Updates the happi update docstring (and by proxy help text) to better describe the use case

Now happi update can take JSON input that looks like a working json backend:

Details

  {
      "al1k2": {
          "_id": "al1k2",
          "active": true,
          "args": [
              "{{prefix}}"
          ],
          <...>
          "type": "pcdsdevices.happi.containers.LCLSItem",
          "z": 778.833
      }
  }

or a list of items in json form (original functionality):

Details

[{
      "_id": "al1k2",
      "active": true,
      "args": [
          "{{prefix}}"
      ],
      <...>
      "type": "pcdsdevices.happi.containers.LCLSItem",
      "z": 778.833
}]

Usage is now described in the subcommand. One can either pipe input and tell happi to look at stdin:

cat my.json | happi update -

or use command substitution

happi update $(cat my.json)

Motivation and Context

#319

Click does not inherently read from stdin, we must explicitly tell it to. If we do this, piping output will work

How Has This Been Tested?

interactively

Where Has This Been Documented?

This PR

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@tangkong tangkong requested review from klauer and ZLLentz July 24, 2023 22:46
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I think this looks correct to me. Is there any obvious way we can unit test this behavior?

@tangkong tangkong marked this pull request as ready for review July 25, 2023 20:17
@tangkong
Copy link
Contributor Author

I'll give unit tests a quick shot, surely there's some stdin mocking in click

Copy link
Contributor

@untzag untzag left a comment

Choose a reason for hiding this comment

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

this works great for me, thanks so much @tangkong

@untzag
Copy link
Contributor

untzag commented Jul 25, 2023

@tangkong consider using subprocess for unit tests? You could set environment variables and use a tempfile for the database itself.

@ZLLentz
Copy link
Member

ZLLentz commented Jul 26, 2023

I think the test looks good, it would have caught this regression

@tangkong tangkong merged commit c9d8248 into pcdshub:master Jul 27, 2023
7 of 9 checks passed
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