MAINT: allow users to import id constants from api

[homosapien-lcy]

Import ids constants in api, allow downstream users to import them from envisage.api.

Closes #506

[mdickinson]

Thank you for the PR! A couple of things:

• It's helpful to always include a PR description. At a minimum, the description should reference the issue that's being solved. (Also, the current settings on this repository are to use the PR title and description for the commit message, so the description is helpful there, too.)
• Like any change to user-visible behaviour, we should have a test. Please could you add a unit test that validates these changes?
• I see that you added two of the ids to envisage.api, but there are still several ids in envisage.ids that aren't being exported in envisage.api. Could you take a look at those ones too, please?

[homosapien-lcy]

Thank you for the PR! A couple of things:

• It's helpful to always include a PR description. At a minimum, the description should reference the issue that's being solved. (Also, the current settings on this repository are to use the PR title and description for the commit message, so the description is helpful there, too.)
• Like any change to user-visible behaviour, we should have a test. Please could you add a unit test that validates these changes?
• I see that you added two of the ids to envisage.api, but there are still several ids in envisage.ids that aren't being exported in envisage.api. Could you take a look at those ones too, please?

Thanks for the comments, I have two questions:

1 which other ids should be included? I actually discussed with a colleague in the same office, we thought that only these two are ids...

2 For the unit test, should I just implement an assert equal for the values imported from api vs from the ids?

[mdickinson]

which other ids should be included? I actually discussed with a colleague in the same office, we thought that only these two are ids...

That's a good question - the goal here is that in the future, all imports from envisage.ids can be replaced with imports from envisage.api. So for the first cut, all the ids should be made available.

For the unit test, should I just implement an assert equal for the values imported from api vs from the ids?

That would work. It's always worth stepping back a bit before writing a unit test and asking exactly what should be tested: we're making a behaviour change for a reason (in this case so that Envisage users can import from api), and the test should reflect the new behaviour - test that the changes have their desired effect.

So in this case, the key thing is to test that these constants are importable from envisage.api - i.e., that doing from envisage.api import <some-constant> succeeds, and gives the expected value. And yes, for that second part, comparing with the value from envisage.ids might be good enough.

[mdickinson]

@homosapien-lcy : Please do add a PR description when you get the chance. (Different projects have different coding standards, but on ETS projects every PR should have a description.)

[homosapien-lcy]

There is a strange problem with ci test... the test runs perfectly in my local Mac:

After looking at the test, it seems to be a unique problem with python 3.7 and 3.8 (my local python is 3.9)... Investigating

[homosapien-lcy]

@homosapien-lcy : Please do add a PR description when you get the chance. (Different projects have different coding standards, but on ETS projects every PR should have a description.)

Got it! Just added

[homosapien-lcy]

There is a strange problem with ci test... the test runs perfectly in my local Mac: After looking at the test, it seems to be a unique problem with python 3.7 and 3.8 (my local python is 3.9)... Investigating

It is caused by a newly merged PR... Now it's fixed.

[mdickinson]

Thanks for the updates! I have a few minor style nitpicks - see line-by-line comments.

Please could you also add the new constants to the docstring at the top of api.py? You should be able to use the :data: Sphinx role for all of them.

[mdickinson]

Style note: on this project, we tend to break up long import lines to place the imports one-per-line. For an example, see the from .extension_point_binding line below.

I'd also like to keep the imports alphabetically ordered as far as possible - this makes it easier for a code reader to scan the set of imports to see if something's there or not (and also helps to minimise merge conflicts in the event that two separate PRs both touch the import list). Please could you move this addition so that it's just before the from .import_manager import?

[mdickinson]

I suggest TestApi, to match the name of the module. (We do have a few existing *TestCase names, but in new code we should migrate away from that pattern.)

Suggested change

	class ApiTestCase(unittest.TestCase):
	class TestApi(unittest.TestCase):

[mdickinson]

Thanks for updating the description! By the way, there's a bit of GitHub magic that's useful here: if you spell the issue reference as "Fixes #506", or "Closes #506", then when this PR is merged, the issue will be automatically closed.

More details here: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

[homosapien-lcy]

Thank you so much! Closes #508

[mdickinson]

Thank you so much! Closes #508

Unfortunately, this only works if it's in the original PR description. (BTW, I think you mean #506 here.)

I've edited the description.

[homosapien-lcy]

@mdickinson The changes you requested have been made

[mdickinson]

@homosapien-lcy Thank you for the updates! I made a quick drive-by fix to the heading underlines. (Otherwise Sphinx will complain at some point.)

I'll merge when the CI completes.

[mdickinson]

Updates look good; thank you.