Donkie / SpoolmanDB

A centralized place to store information about 3D printing filaments and their manufacturers
MIT License
30 stars 19 forks source link

feat: implement #19 #21

Closed StuSerious closed 4 months ago

StuSerious commented 4 months ago

this should cut it, I don't know how can I run the deploy workflows for testing though, do I have to cherry pick this to my main branch? 🤔

StuSerious commented 4 months ago

I was thinking, wouldn't it be better if instead of adding another field we just merged two into one? something like this:

                    "extruder_temp": {
                        "$comment": "Single or range of temperatures for the extruder in degrees Celsius.",
                        "type": "array",
                        "minItems": 1,
                        "maxItems": 2,
                        "items": {
                            "type": "integer"
                        }
                    },
                    "bed_temp": {
                        "$comment": "Single or range of temperatures for the bed in degrees Celsius.",
                        "type": "array",
                        "minItems": 1,
                        "maxItems": 2,
                        "items": {
                            "type": "integer"
                        }
                    },

this shouldn't break existing jsons in theory

Donkie commented 4 months ago

I was thinking, wouldn't it be better if instead of adding another field we just merged two into one? something like this:

                    "extruder_temp": {
                        "$comment": "Single or range of temperatures for the extruder in degrees Celsius.",
                        "type": "array",
                        "minItems": 1,
                        "maxItems": 2,
                        "items": {
                            "type": "integer"
                        }
                    },
                    "bed_temp": {
                        "$comment": "Single or range of temperatures for the bed in degrees Celsius.",
                        "type": "array",
                        "minItems": 1,
                        "maxItems": 2,
                        "items": {
                            "type": "integer"
                        }
                    },

this shouldn't break existing jsons in theory

It will break existing jsons since the values there aren't arrays, they're integers. I prefer not to keep breaking existing norms, so that's why I suggested adding a new separate field.

StuSerious commented 4 months ago

Also, there are quite a few commits fixing validation failures and polishing, do you want me to squash them or should I resubmit with only one commit?

Donkie commented 4 months ago

Don't worry about too many commits, do as you wish in your own branch :)

StuSerious commented 4 months ago

everything should be ok on my side, let me know what you think ;)

StuSerious commented 4 months ago

@YanceyA your feedback is welcome :)

StuSerious commented 4 months ago

image Found a simpler regex pattern, I think it can be simplified more still.

https://regex101.com/r/SedDNb

Donkie commented 4 months ago

Looks good!

Donkie commented 4 months ago

now I'm thinking actually that the material name -CF/-GF additions with the new fill field is redundant. Either of the two ways of specifying fill should probably be removed

YanceyA commented 4 months ago

Hi Donkie, It is my opinion that the fill field and the material post fix [-CFxx / -GFxx] are valuable. Fill provides relevant categorical information that can be used for sorting/filtering or passing this information to other systems for use or display (future use case). The material post fix is required to allow this field to conform well to the ISO 1043 and 11469 material marking standards. The material field is the quickest and best way to understand the polymer construction.

That said, the material field , as long as we follow a format, could be parsed back into fill category (and others) rather simply by someone with the need.

I suggest to keep both but if we only keep one the material post fix is strongly preferred over the fill category.

Donkie commented 4 months ago

Then I suggest we remove the fill field

StuSerious commented 4 months ago

I think we should also consider that 3D Printing materials can have other type of fills that we have not accounted yet, such as wood and ceramic.

I think either way is fine, it's hard for me to see a proper solution.. If we must get rid of a field, then we just suffix the material name as we are doing right now, or we spell it plaintext in the fill field.

your suggestions for implementation are super welcome!

StuSerious commented 4 months ago

Also, I'm working on a readme rework that also documents features added in here.