Skip to content

Conversation

@jsmwoolf
Copy link

Fixes #31

Copy link

@deece deece left a comment

Choose a reason for hiding this comment

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

Looks reasonable, but the nesting looks pretty deep (admittedly, it was deep prior). I think this could be improved by factoring out the core of the loop into another function.

@jsmwoolf
Copy link
Author

jsmwoolf commented Nov 27, 2018

Hopefully, this should improve readability a bit with processing the file. I did two other things:

  1. Since the code doesn't use the arrow function syntax, I decided to keep it consistent with the rest of the code.
  2. I relocated the importbutton.className to before and after the end loop. I removed them from the main logic since the current code only took into account one file. It would be best for the button to unlock after all files have been processed.

@deece
Copy link

deece commented Nov 27, 2018

Looks good to me :) (I have not tested this though)

@jsmwoolf
Copy link
Author

@deece,

Thanks for taking a look at it. Any idea who has permission to merge? I noticed that there's only one merge from August.

@deece
Copy link

deece commented Nov 27, 2018

AFAIK only @Jack000 can merge them

@dorkmo
Copy link

dorkmo commented Nov 27, 2018

should we start a development branch somewhere until jack returns?

@deece
Copy link

deece commented Nov 27, 2018

I'm in favour of that, without it, projects become abandonware. I'm just about to to the same on GerberTools.

Just make sure you add a new issue to point people to where the new code is.

@dorkmo
Copy link

dorkmo commented Nov 27, 2018

Is jack also @bmtm ?

@zaped212
Copy link

Not directly related to the PS.
Is there anyword on creating the development branch? It seems like there is other bugs out there that would benefit from this if we are unable to get things merged into master.

@jsmwoolf
Copy link
Author

Hi @zaped212,

The idea was in favor. However, I'm not aware of any branch being created at the moment. @deece, would one have to fork from this project or is there a way to add a development branch onto this repository?

@deece
Copy link

deece commented Jan 1, 2019

I'll review & merge PRs on my fork until @Jack000 is available: https://github.com/InfernoEmbedded/Deepnest

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.

5 participants