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

Move examples from sdf.md to examples directory #118

Merged
merged 16 commits into from
Sep 26, 2023

Conversation

JKRhb
Copy link
Collaborator

@JKRhb JKRhb commented Sep 24, 2023

As a first step towards the resolution of #33, this PR proposes moving the "full" examples contained in sdf.md to an examples directory, where they are imported from. Based on these changes, a validation pipeline could be set up in a future PR.

For now, only complete SDF models are extracted from the sdf.md file – let me know if it would make sense to also add some of the example fragments as external files.

Besides regenerating (to ensure that the output stays the same), I also added the .refcache directory to the .gitignore file to avoid accidentally committing any reference files downloaded during the rendering process.

@cabo
Copy link
Member

cabo commented Sep 24, 2023

These days, I usually do the inverse, keeping the examples in the .md and extracting them by a Makefile target like

sourcecode: draft-ietf-foo.xml
	rm -rf sourcecode.ba?
	kramdown-rfc-extract-sourcecode -tfiles draft-ietf-foo.xml

But this works for me, too.
If we go the extraction route, it helps to give the examples a name, as in:

{: sourcecode-name="my-example-one.sdf.json"}

@JKRhb
Copy link
Collaborator Author

JKRhb commented Sep 24, 2023

Thank you, @cabo, that looks quite promising :) I tried to incorporate your suggestion via the latest commits. A few questions here, though:

  • Should all of the "fragment" JSON examples also be exported/kept in the sourcecode directory? If so, should they be turned into valid JSON (by adding surrounding curly braces)?
  • Should the CDDL and JSO files outside the document also be removed and only extracted from the Markdown file?
  • At the moment, all dots in the provided sourcecode-names are replaced, leading to exports in the form of example_sdf_json – should these file names be corrected with regard to their file extensions? Or is this unintended behavior of kramdown-rfc-extract-sourcecode?

@cabo
Copy link
Member

cabo commented Sep 25, 2023

  • Should all of the "fragment" JSON examples also be exported/kept in the sourcecode directory? If so, should they be turned into valid JSON (by adding surrounding curly braces)?

The extractor currently does not process the JSON fragments.
The curly braces are actually added for the quick sanity check that kramdown-rfc applies to JSON files, but adding them in the actual document would be distracting, and actually misleading, as the fragments often are not full JSON maps.

  • Should the CDDL and JSO files outside the document also be removed and only extracted from the Markdown file?

I don't think so -- these do not get edited in the .md like the examples do, so editing them externally is the right way. Even more so for the generated JSO files.

  • At the moment, all dots in the provided sourcecode-names are replaced, leading to exports in the form of example_sdf_json – should these file names be corrected with regard to their file extensions? Or is this unintended behavior of kramdown-rfc-extract-sourcecode?

Very good question. Right now the tool is rather conservative on what it allows in a file name; that needs to be adjusted a bit so the sourcecode-name attributes actually survive as file names when chosen reasonably.

Now cabo/kramdown-rfc#206

@JKRhb
Copy link
Collaborator Author

JKRhb commented Sep 26, 2023

Thank you for your feedback and for opening cabo/kramdown-rfc#206 :) I now adjusted the Makefile so that only the named JSON listings from the document are kept in the sourcecode directory.

In this regard, I was wondering if the addition of an option for limiting the source code extraction to listings with a sourcecode-name could be useful? That would give users a bit more control about what is actually being extracted.

@cabo
Copy link
Member

cabo commented Sep 26, 2023

In this regard, I was wondering if the addition of an option for limiting the source code extraction to listings with a sourcecode-name could be useful? That would give users a bit more control about what is actually being extracted.

I'm a bit lazy and simply only check in those parts of the sourcecode directory that I want to have under separate sourcecode control. (You can put unnamed-* into .gitignore ...)

@JKRhb
Copy link
Collaborator Author

JKRhb commented Sep 26, 2023

Oh, very good point, that is definitely the simplest approach here :)

Makefile Outdated
@@ -0,0 +1,9 @@
render: sdf.md
Copy link
Member

Choose a reason for hiding this comment

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

This needs all the included files as dependencies:

   1706:{::include sdf-framework.cddl}
   1721:{::include sdf.jso.json-unidiff}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed now via 8cd0cb7 :)

Copy link
Member

@cabo cabo left a comment

Choose a reason for hiding this comment

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

The Makefile will likely be overwritten once this repo fully adopts the i-d-template.
(And it needs the dependencies I mentioned below.)
Otherwise, quite useful update!

@JKRhb
Copy link
Collaborator Author

JKRhb commented Sep 26, 2023

Thank you for the review and the feedback, I think the PR should now be ready then :) I guess given the commit history, "Squash and merge" would probably be the best way of merging here – or should I squash the commits manually before merging?

@cabo cabo merged commit a451928 into ietf-wg-asdf:master Sep 26, 2023
@JKRhb JKRhb deleted the example-dir branch September 26, 2023 08:51
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