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

Enhancement #317 mark-as-read-backfill #333

Merged
merged 9 commits into from
May 23, 2024
Merged

Conversation

Sakunam
Copy link
Contributor

@Sakunam Sakunam commented Apr 25, 2024

#317

I did the first way you suggested.

@Sakunam Sakunam closed this Apr 26, 2024
@Sakunam Sakunam reopened this Apr 26, 2024
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 97.77778% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.90%. Comparing base (25a7207) to head (8701067).

Files Patch % Lines
src/reader/plugins/mark_as_read.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
- Coverage   95.16%   91.90%   -3.26%     
==========================================
  Files          95       95              
  Lines       11611    11656      +45     
  Branches     2179     1539     -640     
==========================================
- Hits        11050    10713     -337     
- Misses        478      875     +397     
+ Partials       83       68      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sakunam
Copy link
Contributor Author

Sakunam commented Apr 26, 2024

I think something's wrong with /src/reader/_storage/_sqlite_units.py, I ran the type checking with mypy for my commits and the main branch and I get the same three errors.

@lemon24
Copy link
Owner

lemon24 commented Apr 29, 2024

Hi, I'm currently away from a computer. I'll be able to take a closer look at this starting with Wednesday.

Re: typing failures: This is likely caused by a new version of mypy being released, feel free to ignore it (IIRC plugins are excluded from type checking).

docs/make.bat Show resolved Hide resolved
@lemon24
Copy link
Owner

lemon24 commented May 1, 2024

Re: typing failures: This is likely caused by a new version of mypy being released, feel free to ignore it (IIRC plugins are excluded from type checking).

mypy issue fixed in ccf010c, rebasing on top of latest should fix it here too.

Copy link
Owner

@lemon24 lemon24 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, much appreciated!

I added a few comments, mainly around making the new code a bit simpler.

If you're in a hurry, feel free to ignore the comments marked with "Nit:".

src/reader/plugins/mark_as_read.py Outdated Show resolved Hide resolved
src/reader/plugins/mark_as_read.py Outdated Show resolved Hide resolved
src/reader/plugins/mark_as_read.py Outdated Show resolved Hide resolved
src/reader/plugins/mark_as_read.py Outdated Show resolved Hide resolved
src/reader/plugins/mark_as_read.py Outdated Show resolved Hide resolved
tests/test_plugins_mark_as_read.py Show resolved Hide resolved
tests/test_plugins_mark_as_read.py Outdated Show resolved Hide resolved
tests/test_plugins_mark_as_read.py Outdated Show resolved Hide resolved
src/reader/plugins/mark_as_read.py Outdated Show resolved Hide resolved
src/reader/plugins/mark_as_read.py Show resolved Hide resolved
@Sakunam
Copy link
Contributor Author

Sakunam commented May 23, 2024

Sorry I took so long, I was on international travel and it took me some time to figure out the changes. Let me know if there's anything wrong. I'm not entirely sure what's going on with the make.bat

@lemon24
Copy link
Owner

lemon24 commented May 23, 2024

Sorry I took so long, I was on international travel and it took me some time to figure out the changes. Let me know if there's anything wrong. I'm not entirely sure what's going on with the make.bat

No worries, thank you for looking into it. Will be back soon with another commit fix the coverage and the make.bat issue.

@lemon24 lemon24 merged commit f8833db into lemon24:master May 23, 2024
4 of 14 checks passed
lemon24 added a commit that referenced this pull request May 23, 2024
@Sakunam
Copy link
Contributor Author

Sakunam commented May 24, 2024

Sounds good, thanks for everything!

lemon24 added a commit that referenced this pull request May 25, 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.

None yet

2 participants