Bug fixes and features #1

Merged
witten merged 12 commits from deroshkin/novel-stats-fork:deroshkin_branch into master 1 year ago

Hey this is Finn from Bookdun Discord. Here are the promised changes. I tried to comment what I changed in the main file.

Everything should be working and documented, but feel free to message me if you want something clarified.

Hey this is Finn from Bookdun Discord. Here are the promised changes. I tried to comment what I changed in the main file. Everything should be working and documented, but feel free to message me if you want something clarified.
witten reviewed 1 year ago
@ -10,3 +10,3 @@
exact.
Example output:
Example output with no optional data:
witten commented 1 year ago
Owner

By "optional data" here, do you mean "optional flags"? Or are you talking about optional data in the source file?

By "optional data" here, do you mean "optional flags"? Or are you talking about optional data in the source file?
Poster

I meant optional output data, but agreed, optional flags is a better way of phrasing it.

I meant optional output data, but agreed, optional flags is a better way of phrasing it.
witten commented 1 year ago
Owner

Got it!

Got it!
README.md Outdated
@ -46,0 +54,4 @@
chapter 1 Lorem:
203 (drafted)
303 (dev edited)
506 words (total)
witten commented 1 year ago
Owner

Love this feature!

Love this feature!
README.md Outdated
@ -46,0 +67,4 @@
drafted: 336 words (~35%)
dev edited: 385 words (~40%)
total: 946 words
```
witten commented 1 year ago
Owner

Any reason this example is in the installation section? I'd expect it with the example output section above!

Any reason this example is in the installation section? I'd expect it with the example output section above!
witten marked this conversation as resolved
@ -56,0 +85,4 @@
* -a or --act — output act-by-act breakdown of word counts (total only)
* --pp — run markdown pre-processor, this allows for a multi-file input
(e.g. each chapter in its own file), but requires the MarkdownPP python
library.
witten commented 1 year ago
Owner

Don't feel strongly, but why not just make this the default? If you hand it only one file, it'll still work, right?

Don't feel strongly, but why not just make this the default? If you hand it only one file, it'll still work, right?
Poster

I don't feel strongly about this either. Basically I wanted to avoid a mandatory dependency, note that the dependency is loaded only if the flag is set.

I don't feel strongly about this either. Basically I wanted to avoid a mandatory dependency, note that the dependency is loaded only if the flag is set.
witten commented 1 year ago
Owner

Got it, thanks. Makes sense. FWIW, I don't mind additional required dependencies as long as they are (relatively) easy to install.

Got it, thanks. Makes sense. FWIW, I don't mind additional required dependencies as long as they are (relatively) easy to install.
witten marked this conversation as resolved
@ -129,0 +166,4 @@
```yaml
[//]: # This text is completely ignored.
```
witten commented 1 year ago
Owner

Nice!

Nice!
@ -129,0 +173,4 @@
### Multi-file support
Splitting your novel into multiple files is supported using the `MarkdownPP`
witten commented 1 year ago
Owner

FYI looks like MarkdownPP is unmaintained now? https://github.com/jreese/markdown-pp

A quick search does yield some other options: https://github.com/cmacmackin/markdown-include/ and https://github.com/neurobin/mdx_include and https://github.com/simonrenger/markdown-include-lines.

Another option if you didn't want to have to specify an include for each chapter would be to specify all your files on the command-line. E.g., novel-stats *.md. I guess the main thing you'd sacrifice with this approach is ordering—unless there was some metadata in each chapter that specified its chapter number...

FYI looks like MarkdownPP is unmaintained now? https://github.com/jreese/markdown-pp A quick search does yield some other options: https://github.com/cmacmackin/markdown-include/ and https://github.com/neurobin/mdx_include and https://github.com/simonrenger/markdown-include-lines. Another option if you didn't want to have to specify an include for each chapter would be to specify all your files on the command-line. E.g., `novel-stats *.md`. I guess the main thing you'd sacrifice with this approach is ordering—unless there was some metadata in each chapter that specified its chapter number...
Poster

Yeah, I missed that.

Unfortunately, from a quick look, all those modules are meant as extensions for markdown conversion, not sure if you can make them output markdown.

Yeah, I missed that. Unfortunately, from a quick look, all those modules are meant as extensions for markdown conversion, not sure if you can make them output markdown.
witten commented 1 year ago
Owner

Ah, gotcha! Should've looked more closely.

Ah, gotcha! Should've looked more closely.
witten marked this conversation as resolved
@ -135,1 +195,3 @@
novel-stats example.md
2. A 6 file example in the `example` folder with the main file
`multi_file.mdpp`. You can try this one out with
witten commented 1 year ago
Owner

Neat!

Neat!
@ -32,0 +32,4 @@
# -pp flag to allow Markdown Preprocessing primarily to allow multi-file novel formatting
# this is implemented using a temporary file created using python's buit-in tempfile library
import MarkdownPP, tempfile
mdfile = tempfile.TemporaryFile(mode='w+')
witten commented 1 year ago
Owner

Clever!

Clever!
@ -52,0 +64,4 @@
status_by_chapter[chapter_heading][current_status] = count_words(chapter_heading)
else:
current_status = line[len(STATUS_MARKER):].strip('()\n')
status_by_chapter[chapter_heading][current_status] += 0
witten commented 1 year ago
Owner

What does the += 0 do? Maybe should just be = 0?

What does the `+= 0` do? Maybe should just be `= 0`?
Poster

The += 0 is intentional.

If this status hasn't been used in this chapter yet it's no different from = 0, mainly here to initialize.

If this status has been used, it leaves it unchanged.

Didn't want to do a split logic.

The `+= 0` is intentional. If this status hasn't been used in this chapter yet it's no different from `= 0`, mainly here to initialize. If this status has been used, it leaves it unchanged. Didn't want to do a split logic.
witten commented 1 year ago
Owner

Okay, I see now! Is it necessary to initialize though? Like won't the previous status_by_chapter[chapter_heading][current_status] = count_words(chapter_heading) still work even if it hasn't been initialized? Or is the idea that sometimes you want to report statuses even if there are zero words for them?

Okay, I see now! Is it necessary to initialize though? Like won't the previous `status_by_chapter[chapter_heading][current_status] = count_words(chapter_heading)` still work even if it hasn't been initialized? Or is the idea that sometimes you want to report statuses even if there are zero words for them?
Poster

Yep, you're right.

I think the only time that line would do anything is if you put a status change at the end of the file (the way it is in my version would report a 0 for that status), and now that I think about it, deleting that line would be an improvement, in my opinion.

Also same thing if you switch status twice in a row.

Yep, you're right. I think the only time that line would do anything is if you put a status change at the end of the file (the way it is in my version would report a 0 for that status), and now that I think about it, deleting that line would be an improvement, in my opinion. Also same thing if you switch status twice in a row.
witten commented 1 year ago
Owner

Makes sense. I'll remove it after merging!

Makes sense. I'll remove it after merging!
witten marked this conversation as resolved
@ -93,2 +116,3 @@
print(f'{status}: {status_word_count:,} words (~{status_word_count * 100 // total_word_count}%)')
print('total: {:,} words'.format(total_word_count))
print(f'total: {total_word_count:,} words')
witten commented 1 year ago
Owner

Code changes look great! At some point it'd probably be good to switch to "formal" argument parsing (argparse, etc.), but certainly not needed now.

Code changes look great! At some point it'd probably be good to switch to "formal" argument parsing (`argparse`, etc.), but certainly not needed now.
witten merged commit 57724e65df into master 1 year ago
witten referenced this issue from a commit 1 year ago
witten commented 1 year ago
Owner

Thanks for taking the time to make these fixes and improvements! Really appreciate it.

Thanks for taking the time to make these fixes and improvements! Really appreciate it.
The pull request has been merged as 57724e65df.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b deroshkin-deroshkin_branch master
git pull deroshkin_branch

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff deroshkin-deroshkin_branch
git push origin master
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: witten/novel-stats#1
Loading…
There is no content yet.