Bug fixes and features #1

Merged
witten merged 12 commits from deroshkin/novel-stats-fork:deroshkin_branch into master 2021-10-22 20:05:18 +00:00
Contributor

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.
deroshkin added 11 commits 2021-10-22 17:26:02 +00:00
deroshkin added 1 commit 2021-10-22 17:59:14 +00:00
witten reviewed 2021-10-22 18:00:28 +00:00
@ -10,3 +10,3 @@
exact.
Example output:
Example output with no optional data:
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?
Author
Contributor

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.
Owner

Got it!

Got it!
README.md Outdated
@ -46,0 +54,4 @@
chapter 1 Lorem:
203 (drafted)
303 (dev edited)
506 words (total)
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
```
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.
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?
Author
Contributor

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.
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.
```
Owner

Nice!

Nice!
@ -129,0 +173,4 @@
### Multi-file support
Splitting your novel into multiple files is supported using the `MarkdownPP`
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...
Author
Contributor

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.
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
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+')
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
Owner

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

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

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.
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?
Author
Contributor

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.
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')
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 2021-10-22 20:05:18 +00:00
witten referenced this issue from a commit 2021-10-22 20:05:18 +00:00
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.
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
No description provided.