Skip to content

Conversation

kmvanbrunt
Copy link
Member

Fixes #1113
Fixes #1118

Converted persistent history files from pickle to compressed JSON

@kmvanbrunt kmvanbrunt requested review from tleonhardt and anselor June 10, 2021 18:37
@kmvanbrunt kmvanbrunt requested a review from kotfu as a code owner June 10, 2021 18:37
@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #1119 (0901c11) into master (463b318) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1119      +/-   ##
==========================================
+ Coverage   98.48%   98.49%   +0.01%     
==========================================
  Files          22       22              
  Lines        4747     4790      +43     
==========================================
+ Hits         4675     4718      +43     
  Misses         72       72              
Impacted Files Coverage Δ
cmd2/cmd2.py 98.39% <100.00%> (+<0.01%) ⬆️
cmd2/history.py 100.00% <100.00%> (ø)
cmd2/parsing.py 100.00% <100.00%> (ø)

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 5700c57...0901c11. Read the comment docs.

python-version: ${{ matrix.python-version }}
- name: Install python prerequisites
run: pip install -U --user pip setuptools setuptools-scm flake8
run: pip install -U --user pip setuptools setuptools-scm nox
Copy link
Member

Choose a reason for hiding this comment

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

Don't you still need to install flake8?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, nox manages virtual environments and the nox script defines what needs to be installed. This allows us to have a singular definition of what is needed to run a test instead of scattered definitions across multiple scripts.

python-version: ${{ matrix.python-version }}
- name: Install python prerequisites
run: pip install -U --user pip setuptools setuptools-scm mypy
run: pip install -U --user pip setuptools setuptools-scm nox
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to still install mypy?

@@ -1,3 +1,7 @@
## 2.1.0 (TBD, 2021)
* Enhancements
* Converted persistent history files from pickle to compressed JSON
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if someone with an old pickle-based history file upgrades to 2.1.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will print an error and start history from scratch. Since 2.0.0 just came out a few days ago, I doubt this will affect too many people.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the error message to include:

The history file will be recreated when this application exits.

anselor
anselor previously approved these changes Jun 11, 2021
@kmvanbrunt kmvanbrunt merged commit fc2e7a9 into master Jun 14, 2021
@kmvanbrunt kmvanbrunt deleted the json_history branch June 14, 2021 15:28
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.

Convert persistent history file to JSON Non-fatal errors at exit of tests
3 participants