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

Adding examples to Janet api documentation. #242

Closed
wants to merge 9 commits into from

Conversation

erichaney
Copy link
Contributor

Added an example for ++, --, and ->. Let me know if the filenames are not what they ought to be.

@bakpakin
Copy link
Member

bakpakin commented Jan 2, 2025

Thanks Eric, I appreciate the code examples!

Looks like the example file for -> should just be examples/->.janet, no need for the escape. To check, run make; make run and verify that examples pop up in the generated docs.

@erichaney
Copy link
Contributor Author

I hesitated to name the file ->.janet since > and < are not permitted in file names for windows.

But I can see that what I did isn't getting picked up in the generated docs. No biggie. Feel free to close this and I can resubmit with the unescaped file name.

@CFiggers
Copy link
Contributor

CFiggers commented Jan 3, 2025

@erichaney You can just change it in place and the PR will update automatically. 🙂

@sogaiu
Copy link
Collaborator

sogaiu commented Jan 3, 2025

I hesitated to name the file ->.janet since > and < are not permitted in file names for windows.

I also thought this when examining the file naming scheme for examples before.

Some of the printable characters that looked potentially problematic to me for Windows included:

  • /
  • <
  • >
  • *
  • %
  • :
  • ?

(", \, or | are not listed because AFAIK, these cannot be in legal Janet identifiers)

FWIW, I think some of the code that handles symbol to file name conversion currently is here.

My translation of that is:

(def- replacer
  (peg/compile
    ~(accumulate
       (any
         (choice (replace (capture (set "/*%"))
                          ,|(string "_" (0 $))) 
                 (capture 1))))))

(defn- sym-to-filename
  "Convert a symbol to a filename. Certain filenames are not allowed on various operating systems."
  [fname]
  (string "examples/" ((peg/match replacer fname) 0) ".janet"))

May be it makes sense to also handle the following specially?

  • <
  • >
  • :
  • ?

For comparison, this is the scheme I settled on for janet-ref. Some use can be see here.

Removed escape from file name.
@erichaney
Copy link
Contributor Author

I removed the escape from the file name. I'll focus on contributing examples that don't use any special characters for now. 👍

@sogaiu
Copy link
Collaborator

sogaiu commented Jan 13, 2025

I think the evaluation result for this line (line 4 of -.janet) is off:

(- 1.4 -4.5) # -> -5.9

What I get here is:

$ janet
Janet 1.37.1-60d9f977 linux/x64/gcc - '(doc)' for help
repl:1:> (- 1.4 -4.5)
5.9

@sogaiu
Copy link
Collaborator

sogaiu commented Jan 13, 2025

For this line (line 4 of accumulate2.janet):

(reduce2 + []) # -> 0

what I get here is:

$ janet
Janet 1.37.1-60d9f977 linux/x64/gcc - '(doc)' for help
repl:1:> (reduce2 + [])
nil

@sogaiu
Copy link
Collaborator

sogaiu commented Jan 13, 2025

Not sure how much we should be concerned with the following sort of thing, but FWIW, I noticed in some lines that there is no space between the # and the -> (i.e. I see #-> instead of # ->). For the most part, I think elsewhere in the examples (also outside of this PR) there has been use of a space.

For reference, the following have examples of the aforementioned:

(Unfortunately, you may need to scroll to see the relevant bits via the links.)

@erichaney
Copy link
Contributor Author

Thanks for pointing out the issues.

I think I will close this PR and resubmit one file at a time so it is easier to review and track changes.

@erichaney erichaney closed this Jan 13, 2025
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.

4 participants