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

Feature: allow tables to be "refreshed" with a set of data #54

Open
tdwright opened this issue Aug 27, 2018 · 14 comments

Comments

Projects
None yet
3 participants
@tdwright
Copy link
Owner

commented Aug 27, 2018

In some use cases it might be helpful to allow a table to be redrawn with new data. For instance, I may want to grab new data every 5 seconds, clear the console and redraw the table with this new data.

It would be great if a we had a method like table<T>.Refresh(IEnumerable<T> NewData).

For this to work well, we would need to preserve everything except the data. I can think of two areas where this may be problematic:

  1. Column widths - perhaps we'd only preserve these if they'd been explicitly set? Would have to experiment. It might be hard to read a table if the columns keep growing and shrinking.
  2. Calculated columns - we'd need to mark them as computed and store the function, so we could recalculate the values when we get new data.

So it'd be a fair chunk of work, but I think this would be a really interesting addition.

Any thoughts?

@pykong

This comment has been minimized.

Copy link

commented Nov 7, 2018

A related feature: It would be great if we could just add rows.
We work with long running tasks which spill out new data on the fly, thus not all data is available on initial rendering.

Hence it would be great if we could just append new rows to an existing table.
Does the API offer any way to suppress the header generation, as this would make it easy to implement.

@tdwright

This comment has been minimized.

Copy link
Owner Author

commented Nov 7, 2018

@pykong That's an interesting suggestion. Let me better understand your ideal use-case...

  1. You'd write a table, as normal
  2. Receive new data
  3. Create a new table with just the new data
  4. Write this new table without the headers, so it can be appended to the first table

I can see how this would be useful and I think this should be something we support.

In both this new case and the original concept (i.e. table.Refresh()) we'd need some way of carrying some table meta-data (formatting, column widths, etc.) forward. In both cases I think the easiest approach would be to continue working with the same Table object. I think this implies the following:

  • When we call .ToString() on our table, we should be able to pass some argument that fixes/finalises/commits the meta-data in place
  • A method called .Refresh() that accepts a new IEnumerable<T> of the same type and replaces the existing data
  • A method called .Append(), which also accepts a new IEnumerable<T> of the same type, but adds the data to the bottom of the table

In your case, would you want .Append() to immediately return the string representation of the new data?

@pykong

This comment has been minimized.

Copy link

commented Nov 7, 2018

Here is a link to a very hacky implementation of the desired feature based on ConsoleTables
https://snippets.cacher.io/snippet/58da2060292f7cdb44b9

@tdwright

This comment has been minimized.

Copy link
Owner Author

commented Nov 8, 2018

Thanks @pykong. I hope we can make it less hacky by offering a method explicitly for this purpose.

Do you fancy having a stab at this? If not, I'll have a go when I next get a chance.

@pykong

This comment has been minimized.

Copy link

commented Nov 8, 2018

@tdwright I am short on time.

However If
... you refer explicitly to a method for append rows in the Console itself (not to a refresh of the whole table object)
... you implemented a way to suppress the generation of the table header.

Then I could then use that as a starting point.

The functionality that I seek need to give only the table body. Hence both header and the delimiter between header and table body need to be omitted.

@tdwright

This comment has been minimized.

Copy link
Owner Author

commented Nov 8, 2018

@pykong No worries. I'll have a look at this at some point soon. (I'm away next week, so it won't be immediate.)

@maxwell9999

This comment has been minimized.

Copy link

commented Nov 17, 2018

Hi @tdwright, I am rather new to .NET (C#) and open source but saw you list this on up for grabs and think it's something I could tackle. Would love your direction on getting started. I could take a stab at this sometime this weekend.

@tdwright

This comment has been minimized.

Copy link
Owner Author

commented Nov 17, 2018

Hi @maxwell9999,

I like your enthusiasm!

If I were to tackle this, I'd break it into the following discrete tasks:

  • Add an optional bool param to the .ToString() method, which (when true) locks the column widths
  • Refactor the implementation of the calculated columns, so that the function(s) can be applied to new data
  • Add a .Refresh() method that takes a new IEnumerable<T> of data and redraws the table.
  • Add a .Append() method, which also takes an IEnumerable<T> of data, but draws the new table without the header or header separator.

Might be quite a big job, so feel free to push your branch as you go along so that others (e.g. me) can play along.

Good luck!
Tom

@tdwright tdwright added this to the Version 2.0 milestone Nov 18, 2018

@maxwell9999

This comment has been minimized.

Copy link

commented Nov 19, 2018

Ok, I've taken my first look at the code and run through the demos.

My approach will be to start with the Refresh and Append methods first as those are the new functionality, with the other two steps being supporting pieces that can come later.

For Refresh: The idea here is save some of the effort done in initial table creation, right? So I will be adding a non-static method in the Table class which takes in an IEnumerable<T> of data and when called by a table object will modify the data in the table to reflect the new data and throw away the old. I am expecting the user to give me data that matches the old data in terms of type T in regards to columns and things like that, right? So the method will be similar to table creation except that I can skip the setup steps and instead replace them with an input-sanitization-like check to ensure the new data will fit in the table.

For Append: Follow pykong's suggestions above, I will be implementing a method that is very similar to Refresh but instead of replacing any data, it will (after a check to confirm valid data) add to the data and then print the new data as normal except omitting the header and the header separator line. That will make it appear as an append to the already printed table.

Does that make sense to you, @tdwright?

Obviously, this will be a bit easier to discuss once there's some code to look at, so I will try to have some kind of initial solution/prototype push to my fork as soon as possible (hopefully by tomorrow, if not, then Tuesday).

Best,
Maxwell

@tdwright

This comment has been minimized.

Copy link
Owner Author

commented Nov 19, 2018

Hi @maxwell9999,

I think that's the bones of it, yes.

The reason I suggested them in that order is that you'd need to do them first in order to reliably refresh/append. Well, need might be a bit strong...

  • We could get away without fixing the column widths if we don't care about the columns lining up between the old and new data. Less of an issue for Refresh(), but potentially a deal-breaker for Append().
  • We can ignore calculated columns for the time being as long as we're clear that they won't work when we call either of these new methods. My preference would be to keep feature parity, but am also happy for us to take an iterative approach, as long as we get there eventually.

With regards to validating that the shape of the data is the same, we should get this for free via the magic of generics. When we create a Table<T>, that instance retains it's knowledge of what T is. This means we can write our new methods to accept an IEnumerable<T> and be confident that everything will "fit".

Finally, with regards to the Append() method, I think we're both on the same page when it comes to what the output should be. I think we can state that de-duping the data is out of scope of a table rendering utility, so we can assume that whatever we've been passed is new and therefore needs to be written. (This fits with what I understand of @pykong's use case, but perhaps they disagree?)

Anyway, it sounds like you're off to a great start. Have a go at throwing some code together and I'd be very happy to review it.

Tom

@pykong

This comment has been minimized.

Copy link

commented Nov 19, 2018

@tdwright I agree that deduping data is not in the scope of this project. If this was your question after all.

@maxwell9999 Kudos for stepping forward on this.
I am usually short on time but feel free to summon me on any PR so I can have a look into it.

@maxwell9999

This comment has been minimized.

Copy link

commented Nov 19, 2018

I definitely agree that locking the column widths and handling calculated columns is important (as you stated in the original post in this issue) and didn't mean to ignore your suggested ordering. I guess in my mind it just seems easier to implement the new features first and then go back and fix up all the parts of the code that don't support them. It seems like both the width locking and calculation persistence aren't currently necessary; they are additions related to the new methods. And, perhaps we'll discover other parts that need to be fixed up along the way. Unless, you'd like to integrate the new code iteratively, in which case, yes, it does make sense to do the support work first so that when the new feature is introduced it works fully. I was planning to see this through, committing each step along the way and then fixing the issue as a whole with one PR. Would you rather have me PR each incremental change?

And, d'oh, of course, once the table is created the T is no longer generic. Pass it a list of objects type not-T and it will not accept it. Thanks for that reminder.

Appreciate all the other clarifications and encouragement. Hope to have something shortly.

@tdwright

This comment has been minimized.

Copy link
Owner Author

commented Nov 19, 2018

Sorry @maxwell9999, I didn't mean to suggest that you had to do things in the order I suggested. Just wanted to ensure you'd fully grokked why I'd suggested they get done. You clearly do, so the order isn't really important. You've obviously thought it through and I'm happy to proceed in whatever manner you'd prefer. 💯

And I'm more than OK with us working on this on one branch and via one PR. We can keep pushing new changes to the branch and the PR will update to reflect this. In fact, this would be preferable to a whole load of smaller PRs.

You may have noticed that I've included this feature in the v2 milestone. That's because I feel that this (along with #60 in particular) represent a significant chunk of new functionality in the project. I mention this so that you know that I appreciate the scale of this issue, and I'm very willing to be hands-on.

Speaking of being hands-on, I wonder whether we could persuade @Chandler-Davidson to come help us with re-working the calculated columns functionality? Whaddya say Chandler? For old-times sake? 😉

@tdwright tdwright added in progress and removed up-for-grabs labels Nov 23, 2018

@tdwright

This comment has been minimized.

Copy link
Owner Author

commented Nov 28, 2018

Hi @maxwell9999! Hope you're well?

Just wondering how things are going on this issue? If you wanted to get some early feedback, that offer still stands. Or, if you're happy plugging away for the time being, feel free to tell me to bugger off! 😁

Tom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.