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

Decouple juniper #30

Closed
wants to merge 97 commits into from
Closed

Decouple juniper #30

wants to merge 97 commits into from

Conversation

saacg
Copy link
Member

@saacg saacg commented Apr 7, 2017

For code review @portante @SahilTikale @naved001

sarthaksharmalive and others added 30 commits March 9, 2017 11:58
adding test-quads-local.sh from course repo
committing testing.txt for git workflow demo
added testing.txt for git workflow
@portante
Copy link
Member

How does this commit reconcile with @vsemp's three and PRs #18 and #27?

Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Can you resolve the conflicts you have, and see about condensing into a smaller set of comments? 43 is a lot to review individually, and since you only have 13 files changes, it seems a bit much.

This was referenced Apr 10, 2017
@saacg
Copy link
Member Author

saacg commented Apr 10, 2017

@portante just resolved the conflicts, and I had not yet condensed the commits using git rebase -i because I wanted to make sure that this would not affect branches that had been based off of this branch, as these commits had already been pushed. I believe you said you would discuss that with us at a later date? Should I just go ahead and rebase?

@saacg
Copy link
Member Author

saacg commented Apr 10, 2017

@portante to clarify how this pr relates to @vsemp's three and #27 (I closed #18): This branch, class/decouple-juniper, was the main branch for developing the abstraction design. When we began development, we were working with the idea that #39 and this branch would be alternative solutions, and thus we would want to keep them in their own separate branches, as we would never merge them together in master.

#27 was a branch that @Hugh472 had been working off of to make changes to HilDriver.py. However, based on our updated design (see next paragraph), this may now be deprecated anyway and perhaps can be closed?

Based on our last meeting, we decided to progress our abstraction design by breaking down hardware_service.py into two new classes, inventory_service and network_service (never decided on name for this class, just using network_service for now). As this branch has our most up-to-date code for this design, when @vsemp had some ideas about how to structure the break down of functions, I requested that he branch off this branch, and make a pr to it when he was ready, so we could easily identify the new changes and see if there would be any major conflicts. This accounts for pr #43. I am not sure of the purpose of #37, as I thought most of these changes also appear in #43.

I now completely appreciate the importance of having well documented comments and commit messages, especially when making a pr.

@saacg
Copy link
Member Author

saacg commented Apr 21, 2017

closed as it has been superseded by newer changes.

@saacg saacg closed this Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants