Skip to content

Conversation

@alesaggio
Copy link

According to Valentin Y Kuznetsov (cms computing tools expert):

  • the args definition was redundant
  • the metadata_query should contain another "dataset".

Also, are we keeping the exception block? If we don't want to, this is anyway to bear in mind in case of return codes different from zero...


args = ['dasgoclient', '-json', '-format', 'json', '--query', query]
result = subprocess.check_output(args)
args = ['dasgoclient', '--format', 'json', '--query', query]
Copy link
Contributor

Choose a reason for hiding this comment

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

according to dasgoclient -h,

  -format string
        Compatibility option with python das_client, use json to get das_client behavior

so I would default to -json - but the output is not identical: -json gives just the ["data"] part of the -format json. The code that uses it is easy to adjust (actually it gets simpler), but the check in https://github.com/cp3-llbb/SAMADhi/pull/31/files#diff-e068c93f5ddd6d675874177f3be1ef45L58 , should become a simple if len(summary_results) > 1 then, I think.

try:
result = subprocess.check_output(args)
except subprocess.CalledProcessError as exc:
result = exc.output
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's safer to print a message that the query failed and exit, rather than work with the output of a failed query

Copy link
Contributor

@pieterdavid pieterdavid 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 details... see the comments

@alesaggio
Copy link
Author

Thanks a lot, dasgoclient is failing again, hoping it's a temporary problem I'll fix this tomorrow...

@pieterdavid
Copy link
Contributor

Going through the list of open PRs: is this change still needed? If yes, could you address the review comments? If not we may close it.

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