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

feat: implement #19 #21

Merged
merged 15 commits into from
May 28, 2024
Merged

feat: implement #19 #21

merged 15 commits into from
May 28, 2024

Conversation

StuSerious
Copy link
Contributor

@StuSerious StuSerious commented May 23, 2024

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
Copy link
Contributor Author

StuSerious commented May 23, 2024

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
Copy link
Owner

Donkie commented May 23, 2024

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.

filaments.schema.json Outdated Show resolved Hide resolved
filaments.schema.json Outdated Show resolved Hide resolved
@StuSerious
Copy link
Contributor Author

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
Copy link
Owner

Donkie commented May 24, 2024

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

@StuSerious StuSerious marked this pull request as ready for review May 27, 2024 11:37
@StuSerious
Copy link
Contributor Author

StuSerious commented May 27, 2024

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

@StuSerious
Copy link
Contributor Author

StuSerious commented May 27, 2024

@YanceyA your feedback is welcome :)

filaments.schema.json Outdated Show resolved Hide resolved
@StuSerious
Copy link
Contributor Author

StuSerious commented May 28, 2024

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

https://regex101.com/r/SedDNb

Copy link
Contributor

@YanceyA YanceyA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Stu, no notes from me and the changes look right on target to address my comments. Thanks for the work!

Once this in committed, I'll ,at minimum ,take a look at the 3dxtech materials and update accordingly.

@Donkie
Copy link
Owner

Donkie commented May 28, 2024

Looks good!

@Donkie Donkie merged commit 53fff56 into Donkie:main May 28, 2024
3 checks passed
@Donkie
Copy link
Owner

Donkie commented May 28, 2024

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
Copy link
Contributor

YanceyA commented May 28, 2024

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
Copy link
Owner

Donkie commented May 29, 2024

Then I suggest we remove the fill field

@StuSerious
Copy link
Contributor Author

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
Copy link
Contributor Author

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

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.

3 participants