Add Attribute support for Part

[nbes4]

UPDATE: API was reworked, please scroll down for more info

This PR aims to integrate Attribute Entities into the Part API for generators. The API is small, simple and flexible while leaving little room for mistakes by the generator dev (like wrong Unit for Type, e.g. Type.CURRENT for Unit.HENRY) and should tie seamlessly into the existing Entities/Expressions. Furthermore, it could provide a basis for discussion about this feature in general 💪

Need for this feature arose from #127 (comment)

Example:

from entities.attribute import Attribute, AttributeType, MetricPrefix
from entities.common import Value
from entities.device import Manufacturer, Part

max_current_attr = Attribute("Max Current", Value("500"), AttributeType.CURRENT, MetricPrefix.MILLI)
features_attr = Attribute("Features", Value("Suction Cap"), AttributeType.STRING, None)

p = Part("XYZ123", Manufacturer("LibrePCB"))
p.add_attribute(max_current_attr)
p.add_attribute(features_attr)

print(p)

Output:

(part "XYZ123" (manufacturer "LibrePCB")
 (attribute "Max Current" (type current) (unit milliampere) (value "500"))
 (attribute "Features" (type string) (unit none) (value "Suction Cap"))
)

As always any feedback welcome! 😄

[ubruhin]

Nice! 🎉

@rnestler Since you initiated the entity classes, maybe you could take a look at this?

Some random comments from my side:

• The NANO prefix is missing 🙈
• Maybe moving these new classes into common.py since all other common classes are there too?
• It might be handy to optionally pass attributes to the constructor of Part, just like the vertices of the Polygon class.
• Actually the entity classes don't need to be foolproof. Most entity classes don't do any validity checks because CI on library repositories will fail anyway if something invalid is generated so there's not much risk in using the generator wrongly. It would be a huge effort to keep all the validity checks between Python and C++ consistent. As an example, the C++ code doesn't accept all metric prefixes on all units (e.g. kilofarad/gigafarad is not supported). So for the Python API I'd focus on simplicity and usability rather than safety...

[nbes4]

Actually the entity classes don't need to be foolproof. Most entity classes don't do any validity checks because CI on library repositories will fail anyway if something invalid is generated so there's not much risk in using the generator wrongly. It would be a huge effort to keep all the validity checks between Python and C++ consistent. As an example, the C++ code doesn't accept all metric prefixes on all units (e.g. kilofarad/gigafarad is not supported). So for the Python API I'd focus on simplicity and usability rather than safety...

Yes I understand, I guess the only real "foolproofness" is that we infer the Unit based on the Type that is passed into the Attribute constructor so that we can avoid having to pass a Type AND Unit param, which is a nice QoL (imo) for the API but not strictly necessary.

Regarding the unsupported MetricPrefixes, I also noticed that some aren't in the dropdown of the UI, so I guess we could end up with unsupported kilofarad from the Python API if somebody isn't careful 😅 I just found it too tempting from a code-design point of view to use a seperate enum for prefixes and units and later combine them to make up a AttributeUnit for flexibility 🙈

Another way of doing things would be to introduce already prefixed unit enums for all types, e.g.

class Unit(EnumValue):
    def get_name(self) -> str:
        return 'unit'
        
class CapacitanceUnit(Unit):
    PICOFARAD = 'picofarad'
    NANOFARAD = 'nanofarad'
    MICROFARAD = 'microfarad'
    MILLIFARAD = 'millifarad'
    FARAD = 'farad'
    

class CurrentUnit(Unit):
    PICOAMPERE = 'picoampere'
    NANOAMPERE = 'nanoampere'
    MICROAMPERE = 'microampere'
    MILLIAMPERE = 'milliampere'
    AMPERE = 'ampere'
    KILOAMPERE = 'kiloampere'
    MEGAAMPERE = 'megaampere'

and combining them with automatic Type inference in the Attribute constructor, e.g.

# Type automatically gets infered by CurrentUnit
attr = Attribute("Max Current", Value("500"), CurrentUnit.MILLIAMPERE)

Or if we really want to ditch the automatic type/unit inference:

# No protection against mistakes like Type.CURRENT with CapacitanceUnit.NANOFARAD
attr = Attribute("Max Current", Value("500"), Type.CURRENT, CurrentUnit.MILLIAMPERE)

Let me know what you think! 👍

[ubruhin]

To be honest, I'd go with the simplest approach:

attr = Attribute("Max Current", Value("500"), Type.CURRENT, CurrentUnit.MILLIAMPERE)

This variant:

attr = Attribute("Max Current", Value("500"), CurrentUnit.MILLIAMPERE)

is also not bad IMO, but somehow I don't like that it will implicitly assume type string if no unit is provided, though theoretically there could be several unitless attribute types (int, bool, float, ...).

And compared to the currently implemented variant:

attr = Attribute("Max Current", Value("500"), AttributeType.CURRENT, MetricPrefix.MILLI)

the simplest approach of passing type+unit has the advantage that the actual unit is visible in the code (just MetricPrefix.MILLI is missing the important information that the base unit is Ampere).

No very strong opinion though 😉

[nbes4]

I agree with your point. 👌

I reworked the API for simplicty and conciseness. We can now use the simplest approach, e.g.

max_current_attr = Attribute("Max Current 2", Value("500"), AttributeType.CURRENT, CurrentUnit.MILLIAMPERE)
features_attr = Attribute("Features 2", Value("Suction Cap"), AttributeType.STRING, None) # unitless

or a more type-safe and very concise approach (recommended), e.g.

max_current_attr = CurrentAttribute("Max Current 1", Value("500"), CurrentUnit.MILLIAMPERE)
features_attr = StringAttribute("Features 1", Value("Suction Cap")) # unitless

Both are equivalent and generate the the same output! All this while still being super easy to extend for future Attribute types, it is just a matter of adding the right AttributeUnit subclass (with the units supported by the LibrePCB UI) and Attribute subclass (literally 3 lines of code). Also Unitless types are no problem.

Feeling pretty confident with this one 😄

[rnestler]

A few things:

Tests are missing. The example you provided looks like a good test case.

Maybe moving these new classes into common.py since all other common classes are there too?

I think having a separate attribute module is fine as well. Maybe we could even split up the common module in the future to have a bit more structure.

Maybe we should consider using string literal types? https://mypy.readthedocs.io/en/stable/literal_types.html

This would make it a bit easier in the API IMO:

AttributeType = Literal['voltage', 'string', 'current', ...]
CurrentUnit = Literal['microampere', 'milliampere', ...]

max_current_attr = Attribute("Max Current 2", Value("500"), 'current', 'milliampere')
max_current_attr = CurrentAttribute("Max Current 1", Value("500"), 'milliampere')

But maybe the "strongly typed" way with Enums provides better auto completions?

Edit: Also regarding literal types I noticed that we couldn't use the EnumValue base class anymore, so it's probably not feasible.

[nbes4]

@rnestler Thank you for your review! 😄 I added a few tests to the PR.

I think having a separate attribute module is fine as well. Maybe we could even split up the common module in the future to have a bit more structure.

👍

But maybe the "strongly typed" way with Enums provides better auto completions?
Edit: Also regarding literal types I noticed that we couldn't use the EnumValue base class anymore, so it's probably not feasible.

I like the ergonomics of the string literal types, but as you said we would lose the EnumValue base class so I'd argue we stick with the Unit and Type classes, since they are also very easy to use imo. I also added another usability improvement to the API by using a union type for the Attribute constructors value parameter, so it can be used like:

max_current_attr = CurrentAttribute("Max Current 1", "500", CurrentUnit.MILLIAMPERE)

instead of

max_current_attr = CurrentAttribute("Max Current 1", Value("500"), CurrentUnit.MILLIAMPERE)

Let me know if we can proceed with these changes! 🚀

[rnestler]

When CI is fixed this should be good to go. Thanks a lot!

[rnestler]

I squashed some commits which were just fix ups and merged 🙂