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

Added JSON output feature for inheritance tools as per issue #752 #838

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

michael-ford
Copy link

As part of a course project we needed to contribute to an open-source database project so we have attempted to address issue #752 for Gemini. We used the argument format from the query command.

Copy link
Collaborator

@brentp brentp left a comment

Choose a reason for hiding this comment

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

thanks, this looks good. I've made suggestions inline.

dest='format',
default='default',
help='Format of output (JSON or default)'
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If all of the inheritance tools have this option, can you put this into the add_inheritance_args function?

default='default',
help='Format of output (JSON or default)'
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

... into add_inheritance_args and same with below.

has_gts = False
from .gemini_bcolz import gt_cols_types
for i, s in enumerate(self.report_candidates()):
if i == 0:
has_gts = [x[0] for x in gt_cols_types if x[0] in s] or False
print("\t".join(s.keys()))
if self.args.format is 'default': print("\t".join(s.keys()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't use is for string equality. use ==. It will probably always work due to interning, but makes me nervous.

@@ -302,25 +303,40 @@ def report_candidates(self):
yield item

def run(self):
test = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

name this something more descriptive than test please

Copy link
Collaborator

Choose a reason for hiding this comment

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

and define res or a better-named variable below where it's used.

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.

3 participants