coddingtonbear / django-measurement

Easily store, retrieve, and convert measurements of weight, volume, distance, area and more.
MIT License
145 stars 32 forks source link

version 2.0 #8

Closed codingjoe closed 9 years ago

codingjoe commented 10 years ago

I made some major changes for python3 and django 1.7 support. It's not finished yet, but I wanted to to take a look. Stuff I did:

codingjoe commented 10 years ago

seems to work, only django 1.7 and python 2.6 don't go well together, but that is just a travis config issue.

I think all that needs to be done is to remove deprecated code, improve tests and update docs.

coddingtonbear commented 10 years ago

This all looks great, @codingjoe! I've made a couple minor suggestions above, but I don't think any of those are really blockers, honestly. What do you think?

Sorry for the delay in getting back with you on this, I've been a little busy.

syphar commented 9 years ago

talked to @codingjoe, another problem came up.

now I try to delete model X. It fails because the MeasurementQuerySet seems only to contain the fields of the mixin.

Error is something like:

django.core.exceptions.FieldError: Cannot resolve keyword 'x' into field. Choices are: z_field_1, z_field_2

In the choices none of the other fields in Y exist.

codingjoe commented 9 years ago

@coddingtonbear @syphar and I did some major design changes. There will be no backwards compatibility as we went for a single value field. Let me know if you are fine with our changes and I'll merge and publish.

shayneoneill commented 9 years ago

Hows the measurement unit stored in this version? I cant work out how to retrieve if the user had specified inches or mm? This seems to provide a bit of a dilema. The current version straight up craps itself with 1.7, but this seems to jetison core functionality as far as I can tell. Is it possible if the required unit is being silently dropped, to just roll in the 1.7 fix and not the patch that drops the measurement unit storage?

codingjoe commented 9 years ago

Hi @shayneoneill, this version isn't even released yet, for the very reason that we dropped the selected unit. There is no 1.7 fix without the rest, we completely redid the entire code. We could add it tho. But it feels oversized to me.

shayneoneill commented 9 years ago

It kind of limits the usefulness. We sell tools, some in imperial sizes, some in metric sizes, so when its inputed as a metric size, it needs to output as a metric size and so on. I'm kind of racking my brain here as to hacks around it (Such as serializing it out to a new field) , but with 20+ fields with unit conversion it just gets a bit haywire. I just don't see a usecase for the field if it can't remember its input format.

syphar commented 9 years ago

to us the problem was that there is (was) very much complexity in the field, and much things felt like bending/hacking django to make this thing work (one model field -> multiple db fields). When we tried to use it unchanged, things in our codebase immediately broke. Additionally there were things like the measure-type in the db, which duplicated things from the field-definition. And some kind of weird logic to interpret the units.

(there were also problems with two-dimensional units and the form-widget).

To us, it was all this broken things (and hacky code), only for the information which was the original unit (which we didn't need).

shayneoneill commented 9 years ago

What exactly was causing the older version to break? I mean as an interem, is there a patch I can apply to get the older version which stores the units working with 1.7?

coddingtonbear commented 9 years ago

I'm afraid not, @shayneoneill, but do feel free to contribute one. Both myself and @codingjoe have pursued solving this issue (via differing avenues) over the course of the last few months, but neither of us have all that much free time to allocate to this project.

As an aside, I did find an existing project that successfully solves the main problem with the ModelAdmin -- that it requires three database fields to store this value -- and am considering spending a little time to dig through to see how they pulled it off, but given how little time I'm able to allocate, it might be months away.

codingjoe commented 9 years ago

Hi @coddingtonbear, I actually used Jake's code as a guidline to initially fix the bug. But I comes with incopatibilities in older django versions. That's why @syphar had the idea to just simplify the package. Less complexity is easier to maintain. @shayneoneill I must admit, we didn't think about the case you describe. But if you only want to sace the original unit and meter, why don't you just use a regular decimal and a choice field? That is in feels in tune with the Zen of Python.