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

Allow overriding of SlickGrid options #23

Merged
merged 2 commits into from
Aug 10, 2015
Merged

Conversation

snth
Copy link
Contributor

@snth snth commented Jun 26, 2015

I wanted a way to override the forceFitColumns parameter and this PR works for me.

Example usage:

display(qgrid.SlickGrid(grid_data, options={'forceFitColumns':False}))


class SlickGrid(object):

def __init__(self, data_frame, remote_js=False):
def __init__(self, data_frame, remote_js=False, precision=None,
options={}):
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, while I agree that this is a common gotcha, in this case I had thought about it and concluded it's fine. options is only read later (in self.options.update(options)) and not modified, hence I don't believe your concern is an issue here. I thought that this usage is more self-documenting than the standard approach of options=None and then later:

if options is None:
    options = {}

I'm happy to change it to the latter though if you are concerned but I don't think it's necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

@snth I realize the particular usage is safe here, but it's easy for someone to make a change in the future that make it unsafe. I switched over the default to None in #24 at any rate.

@TimShawver
Copy link
Contributor

@snth this looks awesome...such a useful change. Once you address Scott's comment I'll be happy to merge it.

@ssanderson
Copy link
Contributor

@TimShawver if you're happy with the functionality here we can merge and iterate on the specific implementation.

@ssanderson
Copy link
Contributor

@TimShawver @snth I made a secondary PR on top of this one at #24. I think it's the right way forward, but I'd like to get your guys' feedback.

@TimShawver TimShawver merged commit bb41de2 into quantopian:master Aug 10, 2015
richardlin047 added a commit to richardlin047/modin-spreadsheet that referenced this pull request Mar 24, 2021
Signed-off-by: Richard Lin <richard.lin.047@berkeley.edu>
richardlin047 added a commit to richardlin047/modin-spreadsheet that referenced this pull request Mar 24, 2021
Signed-off-by: Richard Lin <richard.lin.047@berkeley.edu>
richardlin047 added a commit to richardlin047/modin-spreadsheet that referenced this pull request Mar 24, 2021
Signed-off-by: Richard Lin <richard.lin.047@berkeley.edu>
richardlin047 added a commit to richardlin047/modin-spreadsheet that referenced this pull request Mar 24, 2021
Signed-off-by: Richard Lin <richard.lin.047@berkeley.edu>
richardlin047 added a commit to richardlin047/modin-spreadsheet that referenced this pull request Mar 30, 2021
…pian#33)

Signed-off-by: Richard Lin <richard.lin.047@berkeley.edu>
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.

None yet

3 participants