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

Added logic to import multiple files when prompted (fixes issues #31) #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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