Skip to content

Rename row -> dataObject, rows -> data #40

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

Merged
merged 4 commits into from
Oct 3, 2018
Merged

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented Oct 3, 2018

To avoid confusion with table rows, the data is internally referred to as "data" (the whole list) and "dataObject" (a single object from that list of data).

The method for setting externally is still called setRows() since externally the table rows are not exposed and this feels logical, but we can always change it to setData if we'd like total consistency.

To avoid confusion with table rows, the data is internally referred to as "data" (the whole list) and "dataObject" (a single object from that list of data).

The method for setting externally is still called setRows() since externally the table rows are not exposed and this feels logical, but we can always change it to setData if we'd like total consistency.
@DanTup DanTup requested a review from devoncarew October 3, 2018 08:47
@DanTup
Copy link
Contributor Author

DanTup commented Oct 3, 2018

Turns out data was used in one place externally (logging) but I hadn't saved the file. So maybe it's a bit weird we call setRows but then do table.data.length. LMK if you think we should rename setRows -> setData (or if you have better names). This is mainly to avoid confusion around what "rows" are in the table virtualisation code (which is somewhat complex).

@@ -92,7 +92,7 @@ class LoggingScreen extends Screen {
}

void _updateStatus() {
final int count = loggingTable.rows.length;
final int count = loggingTable.data.length;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps expose a rowCount getter on Table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call :)

Done, and also moved the constructors to fix the lint. Running "Sort Members" on these files makes a LOT of changes - we may want to do it at some point when there aren't too many PRs open!

Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

I don't normally sort members - does it sort in Flutter order?

Changes lgtm!

@DanTup
Copy link
Contributor Author

DanTup commented Oct 3, 2018

does it sort in Flutter order?

Oh, no - doesn't seem to. It does put constructors before methods, but doesn't put them first. I think one of the codebases I contribute to normally sorts (analysis server?) but I think I'm useless at remembering, so my code normally gets sorted by others!

@DanTup DanTup merged commit 662318f into flutter:master Oct 3, 2018
@DanTup DanTup deleted the renames branch October 3, 2018 16:10
@devoncarew
Copy link
Member

Yes, analysis server sorts, but most everything else leaves things where they are (and Flutter just wants ctors first).

hrajwade96 pushed a commit to hrajwade96/devtools that referenced this pull request Dec 18, 2024
hrajwade96 pushed a commit to hrajwade96/devtools that referenced this pull request Dec 18, 2024
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