reedline icon indicating copy to clipboard operation
reedline copied to clipboard

Sqlite database now gets the session_timestamp from the history session id

Open dasbard opened this issue 1 year ago • 12 comments
trafficstars

(Sub pull request required for a nushell pull request)

Obtains the session_timestamp for the SqliteBackedHistory from the session (HistorySessionId) to avoid desynchronization.

The commented out update_session function on the History trait will be explained in the other nushell pull request.

dasbard avatar Dec 02 '23 03:12 dasbard

CI is mad but should be an easy fix with rustfmt.

When I originally wrote the sqlite sessions stuff, I included the ability to toggle the session. I'm wondering if that would be useful for what you're trying to do here. This is the original PR https://github.com/nushell/reedline/pull/562. Look for toggle history_session. I realize it's a bit different but seeing your commented code for update_session made me think about it.

fdncred avatar Dec 02 '23 13:12 fdncred

I have taken a look at toggle history_session but I have no idea how to properly do something similar inside the nushell evaluate_repl function (most likely) however a similar function to update the history session has been created.

I did run the cargo fmt and clippy commands but for some reason a lot of changes happened and I don't know why. Same with the one commit (https://github.com/nushell/nushell/pull/11206/commits/2dcb8217e14cd1ebd27cd79de64f6799c79b8be2) in nushell.

ghost avatar Dec 02 '23 21:12 ghost

I did run the cargo fmt and clippy commands but for some reason a lot of changes happened and I don't know why. Same with the one commit (https://github.com/nushell/nushell/commit/2dcb8217e14cd1ebd27cd79de64f6799c79b8be2) in nushell.

We probably won't be able to accept this the way it is, marking this draft for now. My guess is you have a rustfmt.toml somewhere that is influencing how it runs.

fdncred avatar Dec 02 '23 21:12 fdncred

Codecov Report

Merging #674 (c089077) into main (93af55c) will not change coverage. Report is 1 commits behind head on main. The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #674   +/-   ##
=======================================
  Coverage   49.19%   49.19%           
=======================================
  Files          46       46           
  Lines        7930     7930           
=======================================
  Hits         3901     3901           
  Misses       4029     4029           
Files Coverage Δ
src/edit_mode/vi/command.rs 30.48% <100.00%> (ø)
src/history/base.rs 83.95% <100.00%> (ø)
src/history/item.rs 44.89% <ø> (ø)
src/history/sqlite_backed.rs 75.08% <100.00%> (+0.32%) :arrow_up:

... and 3 files with indirect coverage changes

codecov[bot] avatar Dec 02 '23 21:12 codecov[bot]

My guess is you have a rustfmt.toml somewhere that is influencing how it runs.

I've been running the command suggested on the Contributing part of the reedline repository' readme in the same directory where I cloned the reedline repository. I don't know why it acted out like this. If you have any idea how I could troubleshoot this, that would be great.

ghost avatar Dec 04 '23 04:12 ghost

@dasbard My only guess is that you have one of these two things going on.

  1. You have rustfmt.toml somewhere. read up on it here https://rust-lang.github.io/rustfmt/
  2. You have some other formatter running in your editor that is changing the formatting.

I haven't seen a user have this problem before so I'm not filled with suggestions. Hope you can figure it out.

fdncred avatar Dec 04 '23 12:12 fdncred

@dasbard My only guess is that you have one of these two things going on. You have rustfmt.toml somewhere. read up on it here https://rust-lang.github.io/rustfmt/ You have some other formatter running in your editor that is changing the formatting. I haven't seen a user have this problem before so I'm not filled with suggestions. Hope you can figure it out.

Turns out I had a rustfmt.toml file inside my home directory for 4 months! I have looked into my~/ .cargo folder before and couldn't find anything and somehow missed that rustfmt.toml could also just be inside ~/

Maybe this could be avoided in the future with an empty rustfmt.toml file inside the repo overwriting user specific format files?

ghost avatar Dec 04 '23 22:12 ghost

Things should finally be fixed. I've also removed the update_session function for the History trait because that seems to not get anywhere and currently has no known usecase as far as I can think.

ghost avatar Dec 04 '23 22:12 ghost

Are there any tests you can write that run in our CI to ensure this feature doesn't get broken in the future? Also there are conflicts.

fdncred avatar Dec 10 '23 01:12 fdncred

I'm unsure about some tests. Maybe I should take a look at it. About that conflict: I clicked on it but I can't seem to select anything to resolve it. I have no idea how to fix it.

ghost avatar Dec 10 '23 12:12 ghost

About that conflict: I clicked on it but I can't seem to select anything to resolve it. I have no idea how to fix it.

it looks like you need to pull the latest main branch and then git merge main in this branch and resolve the conflicts there, then push to the pr again.

fdncred avatar Dec 10 '23 15:12 fdncred

It's been a while and I'm lost as to what problem this PR is fixing. Can you talk more about that and fix the conflicts @dasbard?

fdncred avatar Jan 11 '24 14:01 fdncred

unresponsive

fdncred avatar Apr 23 '24 12:04 fdncred