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

A bit of everything related to v0.3 #80

Merged
merged 10 commits into from
Mar 12, 2022
Merged

A bit of everything related to v0.3 #80

merged 10 commits into from
Mar 12, 2022

Conversation

HugoGranstrom
Copy link
Collaborator

@HugoGranstrom HugoGranstrom commented Mar 10, 2022

Creating this now so I don't forget some parts.
TODO:

  • Missing mustache import
  • nbClearOutput
  • nbRawOutput
  • add readCode parameter to nbNewBlock
  • Write some tests

@HugoGranstrom HugoGranstrom marked this pull request as ready for review March 10, 2022 22:16
@HugoGranstrom
Copy link
Collaborator Author

The implementation is done now. Opinions are welcome. I will add a few tests tomorrow :)

Copy link
Owner

@pietroppeter pietroppeter left a comment

Choose a reason for hiding this comment

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

only one change request, the rest looks good. thanks!

@HugoGranstrom
Copy link
Collaborator Author

Hmm I don't think I can see the change request 🤔

@@ -16,15 +16,16 @@ func nbNormalize*(text: string): string =
text.replace("\c\l", "\n").replace("\c", "\n").strip # this could be made more efficient
# note that: '\c' == '\r' and '\l' == '\n'

template newNbBlock*(cmd: string, nbDoc, nbBlock, body, blockImpl: untyped) =
template newNbBlock*(cmd: string, nbDoc, nbBlock, readCode, body, blockImpl: untyped) =
Copy link
Owner

Choose a reason for hiding this comment

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

readCode should be bool and we could position it after cmd: string. We probably could also add the same old api where readCode is defaulted to true (not sure if template composition runs into issues there...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving it to the front sounds good 👍 I'm a bit afraid of adding types to templates now after battling with them yesterday. (saw that you had also come across this) but as you already have a string there it should be fine. Do you mean to default it as a readCode: bool = true or making a separate overload without it that just calls this new one? That's also something I battled with yesterday as optional parameters aren't supported in templates with untyped arguments without the experimental pragma. So the separate overload is really the only option. (if template composition behaves as we want it to as you said 🤣)

Copy link
Owner

Choose a reason for hiding this comment

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

Yep separate overload. I also had issues with optional arguments in templates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll give it a try after lunch. Feels good to have a test suite in nimib to rely on with these things now :)

Copy link
Owner

@pietroppeter pietroppeter Mar 11, 2022

Choose a reason for hiding this comment

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

Yes that is another boone of recent PR... next step should be automatic generation of documentation with netlify/vercel preview as in scinim/getting-started

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that is pretty neat! Do you prefer nbCode, nbText etc to use the variant with or without the explicit readCode parameter?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes please to which one of them? 😅 Old or new?

@pietroppeter
Copy link
Owner

Hmm I don't think I can see the change request 🤔

weird, cannot find it either. Added it as a single comment

@HugoGranstrom
Copy link
Collaborator Author

weird, cannot find it either. Added it as a single comment

Forgot to "add" it perhaps? Have happened to me before that I write a comment and then click the "Review Changes" button without actually "adding" the comment.

@pietroppeter pietroppeter merged commit 1952ada into pietroppeter:main Mar 12, 2022
@pietroppeter pietroppeter mentioned this pull request Jul 2, 2022
pietroppeter added a commit that referenced this pull request Jul 11, 2022
fixes #85 (last step before tagging and releasing)

- improve changelog
- make sure all important changes (recent and older) are documented

details:

- [x] improving changelog and integrating thanks in changelog
  - [x] 0.1
  - [x] 0.1.x
  - [x] 0.2
  - [x] 0.2.x
  - [x] 0.3
  - [x] move changelog to separate file
- [x] improve documentation for some of the recent changes (from 0.2.0 until now)
  - [x] stuff in 0.2.0 that went un(der)documented
    - [x] add list of command line options generated with `nbInit`
  - [x] changes in 0.2.x
    - [x] 0.2.2: nbFile
  - [x] other 0.3 improvements by Hugo #80
    - [x] nbRawOutput
    - [x] nbClearOutput
  - [x] release 0.3 stuff #81
    - [x] newNbCodeBlock
    - [x] newNbSlimBlock
    - [x] new nbTextWithCode that does read code
    - [x] example files.nim for File
  - [x] nbPython # 83
    - [x] nbInitPython and nbPython
  - [x] nim to javascript #88 - added a section
    - [x] nbCodeToJs (or by pieces nbInitToJs, addCodeToJs, addToDocAsJs)
    - [x] nbKaraxCode
  - [x] CodeAsInSource now the default:
    - [x] remove bullet point in API section
    - [x] add section "Code Capture"
  - [x] make sure to rerun nimble readme to have readme updated (currently missing the interactivity docs)
- [x] other changes
  - [x] turned off warning for unused imports in `nimib.nim` (#103)
  - [x] fix md output of index.nim (bug in markdown backend?)


- some other improvements that could be done later: #110
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.

2 participants