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

Renamed Goods as PhysObj #17

Merged
merged 11 commits into from
Sep 18, 2018
Merged

Renamed Goods as PhysObj #17

merged 11 commits into from
Sep 18, 2018

Conversation

gracinet
Copy link
Contributor

After #16, if feels wrong to use "Goods" for the model representing all physical objects, since some of them are not goods at all (we could even have an Earth or Universe top location among them).

also took the opportunity to correct a few old (and sometimes very
wrong docstrings)
@coveralls
Copy link

coveralls commented Aug 29, 2018

Pull Request Test Coverage Report for Build 225

  • 546 of 546 (100.0%) changed or added relevant lines in 62 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 100.0%

Totals Coverage Status
Change from base Build 224: 0.7%
Covered Lines: 4824
Relevant Lines: 4824

💛 - Coveralls

@petrus-v
Copy link

petrus-v commented Sep 4, 2018

Sounds good to rename. I'm wondering how to manage virtual things likes licenses and so on but as long there is no physical things to represent virtual things I don't believes we needs to follow some stock !

I'm not so aware about all this folk works and have never played with it, I've only give a quick eye to the diff. I'm wondering:

  • if you plan to rename goods namespace en module to physobj as well (probably with some backward compatibility) ?
  • I'm not sure if you make a difference between goods and physObj? does physObj supposed to support GOODS_STATES ?
  • why Avatars still using "goods" m2o attributes (which is not renamed) ?

regards,

@gracinet
Copy link
Contributor Author

gracinet commented Sep 4, 2018

@petrus-v thanks for your remarks !

Let me address them in order:

  • yes, virtual things such as licenses (and services) aren't supposed to be managed by Anyblok WMS It's btw already listed as an example of the differences between PhysObj Types and a concept of Product
  • the core.goods package should indeed be renamed as core.physobj, but that'll be totally transparent except for crossrefs in apidoc, because in the Anyblok world, users don't import code directly
  • the GOODS_STATES constant should be renamed as AVATAR_STATES or PHYSOBJ_STATES. Apart from that PhysObj is indeed just a rename of Goods (performed after Location has been merged in Goods.
  • indeed the Avatar.goods field should now be renamed for consistency (maybe simply as obj, since there's no ambiguity in that context with other ideas of "object")

There's still work to be done on this PR. I may end up writing some migration steps to rename tables and columns (if @jssuzanne asks me to).

@gracinet gracinet mentioned this pull request Sep 4, 2018
@gracinet
Copy link
Contributor Author

gracinet commented Sep 5, 2018

I believe that all comments of @petrus-v are now addressed.
There would be new ones to add, such as the fields of Arrival and Apparition still been called goods_type etc, but I'm not sure if that's to be treated now.

I will indeed add migration steps to issue the relevant ALTER TABLE and ALTER COLUMN statements.

@gracinet
Copy link
Contributor Author

gracinet commented Sep 7, 2018

Build fails because anyblok_wms_base requires a version of Anyblok that's not released yet.
We could switch Travis to GitHub's master branch of Anyblok if needed for the time of this PR (not worth the effort for now, and I think it has value to check on released versions).

Now that Wms.Goods also encompasses the former concept of Location,
a more general naming felt necessary, hence PhysObj, short for Physical
Object.
The constant name and its doc dated back from before the split
of Goods into Goods and Avatars, making it rather confusing, now.
This is of little impact for users, due to direct imports
being uncommon within Anyblok, but this commit renames
Python packages and modules from the 'goods' terminology to
the 'physobj' one.
Comes with compatibility wrappers to ease the transition
There are still various goods_type, goods_properties etc
fields, e.g, in Arrival, RequestItem etc, but somehow it
feels less disturbing and will be done later.
We don't know of any DB in service with reservation data yet, so
we only take care of the data in wms_goods table.

Implemented as a pre_migration, requiring the proper version
comparison of AnyBlok, as well as various fixes related to
these migrations, released in version 0.20.0
There are still lots of variables named 'goods', including
test attributes, but only a few will matter for code
clarity and will be treated later
it is in fact very seldom used: only two occurrences in
wms-core, and a few in wms-quantity.
Now that AnyBlok 0.20.0 is out, featuring SharedDataTestCase,
and since in this branch we require it for other reasons, we
can as well get rid of the compatibility copy we had in
anyblok_wms_base.
@petrus-v
Copy link

Well done !

@gracinet
Copy link
Contributor Author

thanks @petrus-v , lots of cherry picking and squashes in that one :-)

@gracinet gracinet deleted the PhysObj branch November 9, 2018 12:57
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