search

enthought / envisage

Envisage is a Python-based framework for building applications whose functionalities can be extended by adding "plug-ins".
http://docs.enthought.com/envisage/
Other
81 stars 26 forks source link

MAINT: allow users to import id constants from api #508

Closed homosapien-lcy closed 1 year ago

homosapien-lcy commented 1 year ago

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

Closes #506

mdickinson commented 1 year ago

Thank you for the PR! A couple of things:

homosapien-lcy commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

Screen Shot 2023-03-08 at 11 07 59 AM

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 commented 1 year ago

@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 commented 1 year ago

There is a strange problem with ci test... the test runs perfectly in my local Mac: Screen Shot 2023-03-08 at 11 07 59 AM 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 commented 1 year ago

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 commented 1 year ago

Thank you so much! Closes #508

mdickinson commented 1 year ago

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 commented 1 year ago

@mdickinson The changes you requested have been made

mdickinson commented 1 year ago

@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.