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

WireViz 0.3: BOM field Qty changed type to str #252

Closed
amotl opened this issue Oct 22, 2021 · 6 comments
Closed

WireViz 0.3: BOM field Qty changed type to str #252

amotl opened this issue Oct 22, 2021 · 6 comments

Comments

@amotl
Copy link
Member

amotl commented Oct 22, 2021

Hi Daniel,

thanks a stack for releasing WireViz 0.3. On behalf of wireviz/wireviz-web#3, I am working to adjust WireViz-Web appropriately.

Hereby, I would like to report that, apparently, the output type of the BOM Qty field has changed. You will be able to inspect this by looking at the subtle change when comparing BOM's JSON representation v0.2 vs. v0.3.

Those changes seem legit:

  1. The bom_list() function has to be used explicitly
  2. The output gained another column "Id"
  3. The output column "Item" was renamed to "Description"
  4. Output items apparently get sorted now, so "Cable" goes first

However, I wanted to bring the type change on the Qty field to your explicit attention. Maybe you also believe it would be better to have this as float number again, unless any other issue speaks against it?

With kind regards,
Andreas.

@formatc1702
Copy link
Collaborator

#251 will involve a full rebuild of the entire BOM building logic, and I agree that float makes more sense for the Qty field. 👍
Your issue is noted and you are welcome to take a look at the new code once it is ready, to confirm it meets your expectations!

I will add that the BOM code is not yet reworked, so I wouldn't go looking just yet.

@formatc1702
Copy link
Collaborator

I can also envision running wireviz.parse() with a new bom parameter in the return_types argument to give back a nested list with the BOM table... one list item per row (containing a list of the individial fields) with the first entry being a list of the header names.

@amotl
Copy link
Member Author

amotl commented Oct 23, 2021

Hi Daniel,

thanks for your kind response.

I can also envision running wireviz.parse() with a new bom parameter in the return_types argument.

From the perspective of using WireViz as a library, if I may share my thoughts here, and attaching to what has been said at #231 and #253 (comment), I would recommend getting rid of the return_types argument altogether and let this function always return a Harness or WireVizResult instance. In this manner, the type hint annotations (-> Any) can also be improved and everything becomes much clearer.

On this instance, implementing .bom as a real @property method, like .png and .svg, would make perfect sense to me in order to a) save resource usage and b) being able to pull any kind of information on demand, without having to invoke .parse() differently.

Coming from the current state, where I have to invoke

bomlist = bom_list(harness.bom())

in order to get what I want, maybe I can suggest putting bom_list() also as a @property method into the Harness object?

Those are just random thoughts, sharing my two cents. I hope you like it.

With kind regards,
Andreas.

@formatc1702
Copy link
Collaborator

I would recommend getting rid of the return_types argument altogether and let this function always return a Harness or WireVizResult instance. In this manner, the type hint annotations (-> Any) can also be improved and everything becomes much clearer.

I would be 100% in favor of this solution, to give the calling function the control over what properties (.svg, .png, .bom, ...) to keep for processing.

A quick clarification: Are you mentioning WireVizResult as a "synonym" for the returned Harness object? Or are you envisioning something different with this?

@amotl
Copy link
Member Author

amotl commented Oct 23, 2021

A quick clarification: Are you mentioning WireVizResult as a "synonym" for the returned Harness object?

Exactly. Sorry for the confusion.

@amotl
Copy link
Member Author

amotl commented Aug 5, 2022

I think it is safe to close this issue. It was only meant to be a heads up. WireViz-Web will be happy to consume the outcome of #251 when it is ready.

@amotl amotl closed this as completed Aug 5, 2022
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

No branches or pull requests

2 participants