Skip to content

Renaming and deleting component#77

Merged
adrianq merged 5 commits intodevelopmentfrom
renaming_and_deleting_component
Oct 8, 2018
Merged

Renaming and deleting component#77
adrianq merged 5 commits intodevelopmentfrom
renaming_and_deleting_component

Conversation

@ddelpiano
Copy link
Copy Markdown
Member

@adrianq I opened this following the branch renaming request, my bad but i forgot always that.

Also, taken into account the one line command for the debug logging, agreed with you.

Regarding the second point to move the import inside the function I would prefer to keep it global, that function can be called quite often and the import perfomance wise add a cost everytime the function is called, even if the module is cached the first time is imported it is still an extra instruction to be ran every time.

Copy link
Copy Markdown
Contributor

@adrianq adrianq left a comment

Choose a reason for hiding this comment

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

Thanks for this @ddelpiano I always forget about the branch naming as well. No worries :) Everything looks good to me python and js side. I have just committed some code that should fix the Travis build. Waiting for the Travis build to finish before merging.
Something else to consider: these two PRs make the code more stable which is nice but I don't think it provides a good user experience. Just replacing one of the entities when two entities share the same name is a bit weird. At least, we should provide some kind of feedback or even more do not allow the renaming. What do u think? Not saying we should implement this in these PRs but maybe we can create a card and revisit it in the future.

@ddelpiano
Copy link
Copy Markdown
Member Author

@adrianq yeah, that behaviour to have the branches named in that way is something i am used since a bit so I forget all the time, when I will have a chance I will spend a bit of time to revisit the bash script so we can use this or the other name convention.
For the other issue already working on it :) , I think me or Facu opened a card for this related to the name collision.
Thx for the fix for the travis build, I will close the card on trello if that works, fingers crossed.

@adrianq
Copy link
Copy Markdown
Contributor

adrianq commented Oct 2, 2018

Looks like there were a couple of issues behind the travis error (https://trello.com/c/eB0GfSDh/317-fix-travis-builds) Would like to have casper tests running before merging this PR. Will keep you posted!

@adrianq
Copy link
Copy Markdown
Contributor

adrianq commented Oct 3, 2018

I have created a branch in org.geppetto.frontend.jupyter that is a clone of your branch. It contains all the dependencies setting a specific version for each of them.

Travis can create now the environment. However, a couple of Casper tests are failing https://travis-ci.org/MetaCell/NetPyNE-UI/builds/436091591 @ddelpiano can you please have a look at them? I have rebuilt the travis build a few times to double check they were not random failures but they are still happening...

@adrianq adrianq merged commit bcb4a8d into development Oct 8, 2018
@adrianq adrianq deleted the renaming_and_deleting_component branch October 22, 2018 21:40
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.

2 participants