Implement renaming a column

[seancolsen]

Fixes #497

Notes

• This PR was originally Implement renaming a column #870 which contains significant discussion. I opened a new PR because the first one contained too many merge conflicts.
• This is marked as a draft because @pavish and I still need to discuss some implementation strategies around re-fetching store data.

Demos and explanation

• Basic column rename
• Error scenarios when renaming a column
• Usability of renaming a column

Issues

• The records within the column appear blank for a short time during the process of renaming. That's while we're awaiting this.recordsData.fetch() within TabularData.ts. I'm re-fetching the records because that's the easiest way to associate them with the new column name. There are some alternate approaches we could consider to improve this behavior. We could mutate the records store with the new column name. That seems somewhat messy though. We could modify the records API to associate cell values with column id values instead of column names. For now though I think this behavior is acceptable.

• ColumnsDataStore.rename is inconsistent with other methods like add because it does not re-fetch the columns data after it renames. I don't like this inconsistency, but I'm not sure of the best way to resolve it.

  • After we rename a column, we do need to re-fetch the columns data, but we also need to re-fetch the records data and the constraints data. We need to re-fetch the whole TabularData structure, and we don't (and shouldn't) have a way to do that from within the Columns class. So instead of re-fetching within ColumnsDataStore.rename, I'm re-fetching within HeaderCell.svelte.

  • Alternate approaches:

    • Re-fetch within ColumnsDataStore.rename and also call TabularData.refresh within HeaderCell.svelte. That's duplicate fetches. Not great for performance.
    • Re-fetch within ColumnsDataStore.rename and call RecordsData.fetch and ConstraintsDataStore.fetch from within HeaderCell.svelte. Messy because if we change the implementation of TabularData.refresh later on, it won't be apparent that we might also need to also change HeaderCell.svelte.

  • Several weeks ago @pavish and I chatted about the best re-fetch pattern to establish. I initially argued that store functions should never re-fetch and that we should always do re-fetching at the view layer (as I've done in this PR) to give ourselves maximum granularity and control. We discussed several other potential patterns to employ and ultimately agreed to consistently re-fetch within store functions that update data and avoid re-fetching at the view layer. I think we might need to re-evaluate that pattern in light of this new example. As our codebase grows, I predict that we'll continue to need more granular control over the re-fetching behavior than our current pattern will allow.

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the master branch of the repository
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[kgodey]

This isn't related to this specific issue, but are all the patterns that you and @pavish decide on for frontend code being documented somewhere for new contributors and/or future hires?

If not, I would suggest documenting them on the Architecture section on the wiki.

[seancolsen]

Thanks for the reminder @kgodey

I think Pavish and I could both be doing a better job here. I'll try to keep this in-mind more as we have sync discussions that are not captured anywhere. We're planning one for early next week.

In the mean time, I gave some thought to question of what documentation we might already lack. I came up with two ideas and asked Pavish for help within each.

• Document front end coding standards for imports
• Document front end coding standards for .ts vs .d.ts

I'll continue thinking on this and add some more documentation if I can. There's still quite a lot of flux in the front end, especially within our component library, component docs, and css, so at this stage I'm hesitant to start codifying rules for some of that code because it's continuing to shift regularly.

[codecov-commenter]

Codecov Report

Merging #976 (ec34b3c) into master (17e9edc) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #976   +/-   ##
=======================================
  Coverage   93.29%   93.29%           
=======================================
  Files          86       86           
  Lines        3161     3161           
=======================================
  Hits         2949     2949           
  Misses        212      212           

Flag	Coverage Δ
pytest-backend	93.29% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17e9edc...ec34b3c. Read the comment docs.

[kgodey]

Thanks @seancolsen!

There's still quite a lot of flux in the front end, especially within our component library, component docs, and css, so at this stage I'm hesitant to start codifying rules for some of that code because it's continuing to shift regularly.

At this stage, I think it would be most useful for you and @pavish to establish a pattern of documentation into your day-to-day workflows; it's okay if the documentation changes frequently. I think it's harder to remember to document decisions if you only do when it seems final in some way.

[seancolsen]

@pavish This is ready for another look now.

In ec34b3c I'm trying to implement the re-fetching approach we discussed yesterday. I know you said it'd be fine to create a function within TabularData to rename the column, but I figured I ought to try out the final pattern that we're seeking.

One issue with this approach is that dispatch in synchronous, so I can't await it. That means that the UI loading spinner completes too soon. Previously the loading spinner would continue to spin until all stores were re-fetched. After ec34b3c, the spinner completes as soon as the column is renamed but before the stores are re-fetched. I'd say that's a minor but noticeable regression over the column-renaming behavior before I added that commit.

So I also added 23f9392 which changes EventHandler to use async callbacks. This fixes the UI regression described above, but perhaps you might spot other concerns with this approach.

Additionally, in 23f9392 I:

• Changed EventHandler.dispatch so that it throws an error instead of silently returning if it can't find the event. I'm preferring an error in this case because it's a scenario I'd never expect to happen at runtime. Right? If so, I'd rather throw an error so that if we have a bug it doesn't go unnoticed.
• On a similar note, I changed EventHandler.dispatch so that it doesn't catch errors from within the callback function (directing them to the console). This way, if there's an error during re-fetch, that error will bubble up to the UI layer where the user will see it in a toast message.

[pavish]

@seancolsen Nice work! I like the inline edit.

I have a few comments that can be handled before we merge this in.

[seancolsen]

@pavish this is ready for re-review.