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

feat: add __repr__ methods for FluxTable, FluxColumn, FluxRecord #281

Merged
merged 5 commits into from
Jul 9, 2021

Conversation

takluyver
Copy link
Contributor

@takluyver takluyver commented Jul 2, 2021

Proposed Changes

__repr__ methods are a convenient feature for exploring objects interactively - i.e. in an interactive shell or a Jupyter notebook. The default representation of an object basically just identifies the type:

<influxdb_client.client.flux_table.FluxRecord at 0x7f5720b63460>

By defining __repr__, objects can display themselves with more useful information:

<FluxTable: 11 columns, 360 records>
FluxColumn(7, label='_measurement', data_type='string', group=True, default_value='')
<FluxRecord field=counter value=282.0>

There are loose conventions for how reprs are formatted, though I'm not aware of any reference for them. They generally fit on one line, to simplify formatting in containers. If it's practical to fit in all the information to reconstruct the object, the repr should be a code snippet to construct it (as I've done with FluxColumn). If not, the repr usually looks like <ClassName detail>, with whatever details are most likely to convey useful information. I've had a guess at what might be useful for FluxTable & FluxRecord, but I'm happy to revise them.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • pytest tests completes successfully
  • Commit messages are in semantic format
  • Sign CLA (if not already signed)

@takluyver takluyver changed the title Add __repr__ methods for FluxTable, FluxColumn, FluxRecord feat: add __repr__ methods for FluxTable, FluxColumn, FluxRecord Jul 2, 2021
@takluyver
Copy link
Contributor Author

Oh, there is a reference for the convention I described, in the Python docs:

If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment). If this is not possible, a string of the form <...some useful description...> should be returned.
...
This is typically used for debugging, so it is important that the representation is information-rich and unambiguous.

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thanks for your PR 👍

There is a few requirements that should be satisfy before we accept the PR:

influxdb_client/client/flux_table.py Show resolved Hide resolved
influxdb_client/client/flux_table.py Outdated Show resolved Hide resolved
@takluyver
Copy link
Contributor Author

Thanks!

I was trying to ensure that the reprs fit on a single line (of reasonable length), by showing just a summary of the information, rather than the full contents. This isn't strictly required - e.g. pandas objects use multi-line reprs effectively - but I generally consider it good practice. In particular, if you get a list of FluxTables, which seems quite common, it's easier to see if each one sits on a single line.

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

@takluyver, thank you for the explanation, it sounds reasonable. Only two requirements remains:

influxdb_client/client/flux_table.py Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@takluyver takluyver force-pushed the flux-structure-reprs branch from 0d1f333 to 4fa46d4 Compare July 9, 2021 08:46
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thanks again for your PR 👍, we are ready to merge into master.

@bednar bednar merged commit 2a0bd82 into influxdata:master Jul 9, 2021
@bednar bednar added this to the 1.20.0 milestone Jul 9, 2021
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