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

explicitly import using std/ in tempfiles.nim #22851

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

Vindaar
Copy link
Contributor

@Vindaar Vindaar commented Oct 20, 2023

At least on modern Nim tempfiles is not usable if the user has https://github.com/oprypin/nim-random installed, because the compiler picks the nimble path over the stdlib path (apparently).

@Araq Araq merged commit 2b1a671 into nim-lang:devel Oct 20, 2023
17 of 19 checks passed
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from e108780

Hint: mm: orc; opt: speed; options: -d:release
174728 lines; 9.373s; 769.898MiB peakmem

@c-blake
Copy link
Contributor

c-blake commented Oct 21, 2023

It's been most of a decade since patchFile & std/ and all that arose. I ran into some compatibility troubles writing nimp a few years ago and now there is atlas. I also prefer --path-based solutions myself, but we are probably stuck with std/[] forever now.

So, this PR reminds me to ask - is there any reason to not just change all stdlib internal imports to std/[..]? (other than not wanting a commit that touches a zillion files in the VC log). This seems like the kind of thing @ringabout could do in short order. It's not that much noise either in the commit or the source code and (probably?) makes the stdlib better example code which is often one role of stdlibs.

@ringabout
Copy link
Member

This seems like the kind of thing @ringabout could do in short order.

It was rejected before. I suppose we can make a warning, which is disabled by default, that is enforced for the compiler or stdlib.

@c-blake
Copy link
Contributor

c-blake commented Oct 21, 2023

Hmm. I could not find any discussion besides #8024 , It's a bit hard to search for import std without getting a lot of false positives. :-)

To be clear, all I was talking about was that inside the stdlib, all the imports are consistently just std/.. qualified - like this PR, but writ large & all over the stdlib rather than waiting for users to stumble on the kind of issue @Vindaar found and (hopefully) figure it out - merely a "distribution property", not any new compiler feature. { EDIT: Like #17202 but not just for runnableExamples. }

It might also make sense in the test programs. (Also, I didn't mean to over-volunteer anyone for this. If there is receptiveness, I could do all the edits.. it mostly seems like just a lot of text edits.)

@Araq
Copy link
Member

Araq commented Oct 21, 2023

@ringabout feel free to work on it.

@ringabout
Copy link
Member

Sure thing

Araq pushed a commit that referenced this pull request Oct 29, 2023
Araq pushed a commit that referenced this pull request Oct 30, 2023
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