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

Add LegendItem #5229

Merged
Merged

Conversation

@birdsarah
Copy link
Member

@birdsarah birdsarah commented Sep 25, 2016

All pull requests must have an associated issue in the issue tracker. If there

  • issues: fixes #5202 #4526
  • tests added / passed
  • release document entry (if new feature or API change)

Still TODO:

  • Add validation on legend items #5228
  • Remaining deprecations as per #4526
@birdsarah
Copy link
Member Author

@birdsarah birdsarah commented Sep 25, 2016

Have checked with JS API (only works after merging #5219). Here's the output

screen shot 2016-09-25 at 1 43 06 pm

@birdsarah birdsarah added status: ready and removed status: WIP labels Sep 25, 2016
@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

@mattpap that should be added to the dev guide, that's great. Yes the conflicts are trivial. I am fixing those, making the few changes you suggested, then I was going to add support for auto-converting the "list of tuples" in the property for .items

bryevdv added 2 commits Oct 3, 2016
@bryevdv bryevdv added status: ready and removed status: WIP labels Oct 3, 2016
@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

@mattpap @birdsarah there are possibly some minor issues but I think this is ready for now with some followup issues:

  • I had to change the taylor_server.py example, creating a new LegendItem with existing renderers inside was not deserializing correctly. But I think that is possibly a weird case in general with server comms. In any case it's work-aroundable as seen.
  • I'm not sure about the JS side LegendItem logic. It's also work-aroundable. I had to add value when I didn't think I'd need to
expect(field).to.be.undefined

it "should return undefined if label property is value", ->
legend_item = new LegendItem({'label': {'value': 'milk'}})

This comment has been minimized.

@mattpap

mattpap Oct 3, 2016
Contributor

This code was copied, so no blame here, but I would like to point out that it's preferred it use name literals instead of strings when referring to members of an interface, i.e. new LegendItem({label: {value: 'milk'}}). This states the intent better, because, after all, label and value are members of some interfaces and we aren't building an arbitrary dictionary here. Currently implicit, but when we will use TypeScript, it will be defined somewhere. Actually, it is already under api/typings. This is similar to what we do in Python when constructing keyword arguments with dict(name=value), instead of {"name": value}.

This comment has been minimized.

@bryevdv

bryevdv Oct 3, 2016
Member

@mattpap that makes sense, I can push updates to those as well. Do you have any other comments on this PR?

return str(self)


DEP_MSG_0_12_3 = """

This comment has been minimized.

@mattpap

mattpap Oct 3, 2016
Contributor

@bryevdv, deprecation warnings have to be changed the new style.

@mattpap
Copy link
Contributor

@mattpap mattpap commented Oct 3, 2016

It's also work-aroundable. I had to add value when I didn't think I'd need to.

Is this an issue with js examples or test?

bryevdv added 2 commits Oct 3, 2016
@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

Is this an issue with js examples or test?

Actually I think it just needs to be expicit for models examples. The nice convenience checking for field of value is part of the plotting API

@mattpap
Copy link
Contributor

@mattpap mattpap commented Oct 3, 2016

LGTM. Any inconveniences will emerge sooner than later after merge.

@birdsarah
Copy link
Member Author

@birdsarah birdsarah commented Oct 3, 2016

@bryevdv @mattpap, thanks for taking this PR the last mile - have looked at all the commits that @bryevdv made and they look good - I'd forgotten about "accepts" so that was much easier than I thought it was going to be.

@mattpap, really appreciating the explanations on PR comments I'm learning a lot and hopefully you won't have to repeat yourself in the future.

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

OK I will merge this when green, I'll make issues for the other small questions I ran into.

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

I can't get the py27 unit tests to pass, the port in use error seems persistent. I wish I knew what changed on travis or whatever else to start this problem.

@mattpap
Copy link
Contributor

@mattpap mattpap commented Oct 3, 2016

@bryevdv, is this 5006 port in use error? I complained about this on internal mailing list. This error comes and goes, so you can restart a dozen times with no result and then after an hour or two tests will start to work again.

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

Yes, it is the same you mentioned. What is odd is that it is always the same test test_log_stats

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

trying specifying a different port in ManagedServerLoop (which BTW is used all over the tests, its very strange that only this one test has issue)

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

that just kicks the can, now a different test is failing the same way. It's always just one.

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

It's also only python 2.7

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Oct 3, 2016

OK, --rerun "worked", will merge when green

@bryevdv bryevdv merged commit 8df284b into master Oct 3, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bryevdv bryevdv deleted the birdsarah/5202_4526_use_legend_items_fix_merging_fix_up_api branch Oct 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.