-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add support for literate programming. #3747
base: main
Are you sure you want to change the base?
Conversation
Adds support for Markdown, reStructuredText and LaTeX files as Swift sources.
@@ -397,6 +400,9 @@ public struct TargetSourcesBuilder { | |||
} | |||
} | |||
|
|||
// Ignore README files. | |||
if path.basename.hasPrefix("README") { continue} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of this; I added it because not having it here broke the build (because of a package that was written in C++ but had a README.md
file in the source directory).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think that it is clear cut that readme files in a target are never supposed to be resources and as such ignoring them doesn't seem right. I am curious how these files could have broken the build, unhandled files should only emit warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, it is because we're saying .md
files are now sources, but if they aren't specially formatted, they will break the build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It's because there's a package written in C++ that has a README.md
in its directory, so the package manager code refuses to build it because it thinks it is a mixed-language package.
The literate stuff doesn't care at all about whether or not there is actually code in a Markdown file.
I'm not certain what the right fix is here, though I am certain that this is particular "fix" is a hack and we should do something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, because .md
is becoming a source file type, it is seen as mixed source.
Would it make sense to have a more specific file extension for the literate programming stuff, so that we can distinguish them from "regular" markdown files? In principle, it could be correct that it is mixed source if the .md file does contain code.
Another approach could be only treating .md files as source if they're part of a target that isn't a C-family one, but that seems awfully specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Piggy-backing on what @neonichu just said: What if we used a compound file extension convention. For example, README.swift.md
(or maybe README.md.swift
?). That way, we wouldn't have to rely on any assumptions or heuristics to determine whether a given file is a conventional text document or a literate programming notebook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense to me. Using plain .md
as files to be compiled seems that it would surprise a lot of users.
@@ -397,6 +400,9 @@ public struct TargetSourcesBuilder { | |||
} | |||
} | |||
|
|||
// Ignore README files. | |||
if path.basename.hasPrefix("README") { continue} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we accommodate differences in case (e.g. readme.md
)?
if path.basename.hasPrefix("README") { continue} | |
if path.basenameWithoutExt.uppercased() == "README" { continue } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. I'm a bit uncertain that this is the right thing to do in any way whatsoever. Maybe it would be better to detect cases where there are C(++)/ObjC(++) sources and ignore any of the file formats we support for literate Swift.
@@ -397,6 +400,9 @@ public struct TargetSourcesBuilder { | |||
} | |||
} | |||
|
|||
// Ignore README files. | |||
if path.basename.hasPrefix("README") { continue} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, to also accommodate variations like readme-en.md
if path.basename.hasPrefix("README") { continue} | |
if path.basename.uppercased().hasPrefix("README") { continue } |
@@ -529,7 +535,7 @@ public struct FileRuleDescription { | |||
.init( | |||
rule: .compile, | |||
toolsVersion: .minimumRequired, | |||
fileTypes: ["swift"] | |||
fileTypes: ["swift", "md", "rst", "tex"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new file types should get their own distinct rule with tools-version being 5.6
@al45tair Do you need any additional information to make a further progress on this? |
@elsh No, but thanks for asking. This work was all somewhat experimental (which is why all the PRs are in Draft). Whether we'll land this in this form, or whether we'll eventually do something else I don't know at this point. |
Adds support for Markdown, reStructuredText and LaTeX files as Swift sources.