EAGLE-1231: Added 'encoding' attribute to fields in Logical Graph schema #265
Conversation
Very small change, but seems to work now again
Added parser fields to doxygen of apps
The method has been split apart into 4 individual methods and the rest has then been integrated with the calling method ‘make_single_drop’. Much more clean now, but required a number of changes throughout the code to make all the tests and the graphs working again. Updated doxygen strings to get the new dropclass parameter for every builtin drop. ‘dropclass’ replaces all the individual *class (e.g. dataclass, appclass) parameters and made quite a bit of the code more straight forward. This had effects in many places around the code. Updated categoryType and category to use both of them consistently throughout the code. Changed to “name” everywhere. There had been a few places where this was not yet done. Cleaned-up the construction of drop_spec, tried to use a dataclass, but that would have caused too many changes for little benefit. adjusted all test graphs adjusted all tests
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
We have skipped reviewing this pull request. We don't currently review these file types ['.schema'] - Let us know if you'd like us to change this.
| "encoding": { | ||
| "type": "string", | ||
| "enum": ["binary", "pickle", "dill", "npy", "base64", "utf-8"], | ||
| "default": "utf-8" |
There was a problem hiding this comment.
I'm assuming this default is a decision that's been made previously, as opposed to being arbitrary?
There was a problem hiding this comment.
I agree in principle not to use utf-8. We did not have the ability to specify this per field, but only globally. The previous global default value was pickle, but I think we should switch to dill instead, since that can serialize a lot more than pickle. Also note that this only applies (like before) if a field is used as a port.
There was a problem hiding this comment.
OK, I'll remove "utf-8" from the options, and set the default to "dill".
Sorry, there obviously is a misunderstanding, what I meant is not to use utf-8 as the default, not to not have it at all. Could you please add it to the list again, since that is the only way to pass plain strings correctly. |
|
@awicenec I've added 'utf-8' back to the options. And 'dill' is the default in the schema. I'd be happy to merge this PR. I'm just a little concerned that github thinks it includes 4819 commits! Would this be sorted out by the merge? |
|
@james-strauss-uwa If you modify the merge branch from |
|
@myxie Thanks for the suggestion, but it didn't seem to work. Do you think we should merge anyway, or (since it is such as small change) create a new branch and PR? |
|
@james-strauss-uwa on reflection the issue is due to the
The issue is that the force push has been applied whilst other branches were using the pre-existing commit IDs. |
|
Created a new branch eagle-1231-2 and will create a new PR for that branch |
Added a new encoding attribute to each field in the Logical Graph schema. Attribute is supported by EAGLE.
Allowed values are:
Default value is "dill"