ChartsWidget for common multi chart usage

[tumaysem]

This for pre-review Remaining tasks I am working on now

1. Separate Line and Scatter plots
2. Comments
3. isort & black

see multi_agent_simulation_chart.py for implementation and asyncio run fix.

[tumaysem]

Thanks @anirudh2, This is a great chance for me to understand the commit format and workflow with this draft PR. Though a little clarificiation I may need 🦆 I believe for a real PR, please correct below steps

1. Open issue for new chart feature
2. Create branch for it
3. Add the chart.py and commit
4. Update the multi_agent_simulation.py commit
5. Create PR & assign for review
6. Merge , and delete branch

[anirudh2]

Thanks @anirudh2, This is a great chance for me to understand the commit format and workflow with this draft PR. Though a little clarificiation I may need duck I believe for a real PR, please correct below steps

1. Open issue for new chart feature
2. Create branch for it
3. Add the chart.py and commit
4. Update the multi_agent_simulation.py commit
5. Create PR & assign for review
6. Merge , and delete branch

It's not so much to do with the above steps. The problem comes from your PR both a) moving code around, and b) refactoring. Those are at least two distinct steps, and, therefore, two distinct PRs. Also, the leap between unrefactored code and refactored code should be incremental, so you probably have multiple PRs there. In general, I try to split up my work into the smallest possible units, each of which would still work. Each one of those then becomes a PR. In my code, this often means that I'm committing each time I write a new function or something similar, since a function is immediately useful, even if it's not actively being used.

[tumaysem]

@anirudh2 I think you mean more iterations/commits. Unfortunately, I've directly wrote the 500 lines of code in 2 steps as first one to wrap the functionality which was already there and next redefine types & documentation.

[anirudh2]

Nope. I mean more PRs. Incremental change is the name of the game. At each incremental change, you get it approved. That's how you don't end up with 500 lines of code that no one sees coming until the PR comes in. If you submit more frequent PRs with fewer lines of code, they become easier to review and to have a discussion about.

[tumaysem]

@anirudh2 how about below PRs

1. Separate charting functionality from multi agent simulation (move)
2. Refactor multi agent simulation charting to remove duplication (change)
3. Wrap multi agent charting into structures to simplify bootstrapping/configuration (change)
4. Refactor missing graphics parameter types (change)

PS: I have opened a single enhancement issue for above PRs, do I need to open multiple issues for each PR ?

[anirudh2]

@aasseman Tbh, I've not looked at what Tumay is refactoring to know if his plan is reasonable. Do you have an opinion?

@tumaysem I'm of the opinion that each issue should encompass only a single topic of discussion. Again, I don't know enough about the code you're refactoring to comment, so I'll defer to @aasseman

[tumaysem]

I have another question about this item "PRs that move code should not also change code".
I will be moving "chart functionality" out of "multi agent simulation" and if we assume each commit should be a working one. I can't think of a way accomplishing this without creating a change on the "multi agent simulation". Even this raises additional shared state design requirement between the "multi agent simulation" and moved code "chart functionality".

[anirudh2]

Good question. Let me give an example using the Gilded Rose kata. If you've not seen this before, this is a classic refactoring kata - a short (< 20 minute) exercise for practicing good refactoring practices.

class GildedRose(object):

    def __init__(self, items):
        self.items = items

    def update_quality(self):
        for item in self.items:
            if item.name != "Aged Brie" and item.name != "Backstage passes to a TAFKAL80ETC concert":
                if item.quality > 0:
                    if item.name != "Sulfuras, Hand of Ragnaros":
                        item.quality = item.quality - 1
            else:
                if item.quality < 50:
                    item.quality = item.quality + 1
                    if item.name == "Backstage passes to a TAFKAL80ETC concert":
                        if item.sell_in < 11:
                            if item.quality < 50:
                                item.quality = item.quality + 1
                        if item.sell_in < 6:
                            if item.quality < 50:
                                item.quality = item.quality + 1
            if item.name != "Sulfuras, Hand of Ragnaros":
                item.sell_in = item.sell_in - 1
            if item.sell_in < 0:
                if item.name != "Aged Brie":
                    if item.name != "Backstage passes to a TAFKAL80ETC concert":
                        if item.quality > 0:
                            if item.name != "Sulfuras, Hand of Ragnaros":
                                item.quality = item.quality - 1
                    else:
                        item.quality = item.quality - item.quality
                else:
                    if item.quality < 50:
                        item.quality = item.quality + 1

class Item:
    def __init__(self, name, sell_in, quality):
        self.name = name
        self.sell_in = sell_in
        self.quality = quality

    def __repr__(self):
        return "%s, %s, %s" % (self.name, self.sell_in, self.quality)

Here, what I might do is identify one thing that I want to refactor at a time - say "Aged Brie" because that's the shortest one to type haha.

PR 1

I'd try to identify what is happening to an Item with name "Aged Brie" as it passes through the update_quality() method. Based on this, I'd write some unit tests to confirm my understanding of the behaviour if they aren't already implemented.

PR 2

Then, I'd implement a method to handle the aged brie case. The code here would not necessarily be the best code, but I'd make sure it passes the tests.

PR 3

Without creating any new objects, I'd now try to clean up the aged brie method a bit, at each refactor making sure I'm still passing the tests. Of key importance while refactoring and implementing new code is the software engineering principle KISS. I've also not yet modified the update_quality() method.

PR 4, 5, and 6

Repeat for "Sulfuras"

PR 7, 8 and 9

Repeat for "Backstage"

PR 10, 11, and 12

Repeat for base case

PR 13

Refactor update_quality() to use new methods, which I have confidence in since I've tested them throughout.

PR 14 - N

See if you can make your solution simpler by creating new objects or helper methods. For each new object or helper, write tests.

At each point, the only time I create a new object or method is to make the code cleaner. I never add something because it's a more clever solution. KISS is one of the driving principles behind good design because it makes your code easier for others to follow and for you to debug. It's always worth remembering this classic quote from Clean Code.

Note

Personally, I'm a fan of trunk-based dev, so when doing that, you'd replace "PR" above with "commit." In either case, the idea is add to the main repo frequently, with small changes. This 1) prevents others from duplicating your effort accidentally since they don't know you're working on it 2) ensures people are able to follow your changes (and revert them) easily and 3) enables us to fail fast. Say you and I are working on the same codebase, and I write some code that causes your code to fail. I'd rather know about that after implementing 10 lines of code than after implementing 1000 lines of code. It's easier for me to narrow down where the problem is coming from in the former case. It's also easier for the repo maintainer to revert my small commit without hindering other people's progress. There are other reasons to favour trunk-based dev imo, but I'll leave it there for now.

[anirudh2]

To demonstrate that I'm actually following this practice myself, here's a PR I opened on another repository that literally just removes an unused type variable as part of a larger refactoring effort. Less than a 1-line change, but it's a standalone change. Therefore, it's a unique PR.

[tumaysem]

I start working on the real PRs, thank you @anirudh2 for your patience and support.

[tumaysem]

Also closing the draft PR thread and deleting related branch.