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

Incorrect package computation #354

Open
sorawee opened this issue Dec 3, 2022 · 8 comments
Open

Incorrect package computation #354

sorawee opened this issue Dec 3, 2022 · 8 comments
Labels

Comments

@sorawee
Copy link
Contributor

sorawee commented Dec 3, 2022

https://docs.racket-lang.org/2d/ shows that #lang 2d is a part of the 2d-test package. It should actually be 2d-lib.

The root of the problem is that defmodulelang (and/or defmodule with #:lang) should not treat the input name as a final module-path to be used. In this particular case, (require 2d) loads 2d/2d-test/main.rkt, which doesn't exist and is in an incorrect package. The correct module path to require should be (require 2d/lang/reader), which loads 2d/2d-lib/lang/reader.rkt in 2d-lib.

@sorawee
Copy link
Contributor Author

sorawee commented Dec 3, 2022

But also, should 2d-test really have (define collection "2d")? Usually it's 'multi?

@sorawee sorawee added the bug label Dec 3, 2022
@rfindler
Copy link
Member

rfindler commented Dec 4, 2022

But also, should 2d-test really have (define collection "2d")? Usually it's 'multi?

I think 2d-test's setup means that it has collection paths like 2d/tests/cond-test, which seems right.

This doesn't contradict your conclusion about the docs, but just in case: I don't think that (require 2d) loads 2d/2d-test/main.rkt per se; that's just the last place it tried and then it gave up. It didn't find a main.rkt in any of the places where 2d/ could take it. If there had been a file main.rkt in the directory 2d/2d-lib/ in the 2d git repo, it would have loaded that one, which seems right. Perhaps this means the error message should be improved?

@sorawee
Copy link
Contributor Author

sorawee commented Dec 4, 2022

It's still unclear to me if this is the fault of defmodule or the fault of 2d-doc.

In 2d-doc, we can change defmodulelang to

@defmodulelang[<something> #:module-paths (2d/lang/reader)]

This correctly determines the package, but what should go in <something>? Also, the change makes links broken in two ways:

  • @racketmodname[2d] no longer links. This can be probably be replaced by something like @racketmodlink[2d/lang/reader]{@racketidfont{2d}}.
  • But more importantly, #lang 2d racket in @codeblock no longer links, and it is unclear how to fix this issue.

@sorawee
Copy link
Contributor Author

sorawee commented Dec 4, 2022

Perhaps this means the error message should be improved?

I agree.

I think 2d-test's setup means that it has collection paths like 2d/tests/cond-test, which seems right.

I notice that for most, it's usually something like tests/2d/cond-test, not 2d/tests/cond-test. For example:

@sorawee
Copy link
Contributor Author

sorawee commented Dec 4, 2022

Another thing is, how much magic do we want from defmodulelang/defmodule #:lang? Ideally, I can write 2d, and it should try both (submod 2d reader) and 2d/lang/reader?

@rfindler
Copy link
Member

rfindler commented Dec 4, 2022

I notice that for most, it's usually something like tests/2d/cond-test, not 2d/tests/cond-test

I think it's a mixture. I see framework, mrlib, redex, 2htdp, htdp, images, math, plot, simple-tree-markup, and optimizer. It may be that the other way is more popular, tho.

I do agree that if the documentation needs to look for language, it should be looking for both the submod and the lang/reader variant. I'm not sure what's the right approach in general here, however.

@sorawee
Copy link
Contributor Author

sorawee commented Dec 4, 2022

My feeling is that the right approach would not be backward-compatible.

Here's an example. @defmodulelang[at-exp] is tagged with (mod-path at-exp) See e.g. the URL https://docs.racket-lang.org/scribble/reader-internals.html#%28mod-path._at-exp%29

But at-exp is not a valid module path. We can't require it. Ideally, it should be changed to (lang-path at-exp) or perhaps (mod-path at-exp/lang/reader). However, that would break the link.

Is it OK to break links?

@rfindler
Copy link
Member

rfindler commented Dec 4, 2022

I think that there are a bunch of languages were #lang XYZ and (require XYZ) both provide the same (or a similar) set of names, so the current setup is probably trying to accommodate those. What if there was another option to @defmodulelang that languages like at-exp could use to indicate that the require form isn't supposed to work? Then for languages like slideshow and racket this extra argument wouldn't be used and they'd keep the status quo (links and all). Or maybe the default behavior would be to have two tags, one for mod-path and one for lang-path so that @racketmodname could go to mod-path and a new @racketlangname could go to the lang-path link.

This would still be backwards incompatible for some of the links (like at-exp) but maybe we could keep a few of the more popular links in place (via yet another option to @defmodulelang)? Or if there aren't that many of these we could just assume that no one is using those links? I guess we could grep the code on the pkg server to see if anyone is writing @racketmodname with at-exp or any of these others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants