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

Implement the new path search algorithm #2788

Merged
merged 1 commit into from
Oct 6, 2016
Merged

Conversation

asterite
Copy link
Member

@asterite asterite commented Jun 9, 2016

This fixes #2690.

The new rules are:

For require "foo", search:

  1. foo.cr (to find something in the standard library)
  2. foo/src/foo.cr (to find something in a shard)

For require "foo/bar" search:

  1. foo/bar.cr (to find something in the standard library)
  2. foo/src/bar.cr (to find something in a shard, non-namespaced structure)
  3. foo/src/foo/bar.cr (to find something in a shard, namespaced structure)

For require "foo/bar/baz" search:

  1. foo/bar/baz.cr (to find something in the standard library)
  2. foo/src/bar/baz.cr (to find something in a shard, non-namespaced structure)
  3. foo/src/foo/bar/baz.cr (to find something in a shard, namespaced structure)

We support rules 2 and 3 in the nested path search so that current shards continue to work, and later we can decide if we prefer to keep this nested structure or not. Or maybe we can leave it like that and each shard author can choose this.

To move forward with this we'd need shards to put the whole contents of a shard inside libs, not just the src directory. I guess this will be easier, but it needs to happen in sync with the next release. @ysbaddaden Would you like to do that?

Additionally, it should probably put things inside lib so we can fix #321 . This needs no change in the compiler, just a change in omnibus.

This is the reason I un-nested many files in the standard library, because the old foo finds foo/foo.cr doesn't apply anymore.

@ysbaddaden
Copy link
Contributor

shards [must] put the whole contents of a shard inside libs

I'll do that over the weekend.

it should probably put things inside lib

Yes, let's do that at the same time.

the old foo finds foo/foo.cr doesn't apply anymore

Oh, it was a nice touch 😢

@asterite
Copy link
Member Author

Oh, it was a nice touch 😢

Yes, maybe, but it only applied to the standard library. Without that rule it's now easier to see which top-level names are occupied by the standard library: all files under src. Previously one also had to check for directories that had a file with their name.

With that, I'd like to free some names. For example event and many others should probably be inside a Crystal::Runtime module or similar.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Sep 25, 2016

This is now implemented in Crystal, and can be merged as soon as it's reviewed and this PR is merged :)

I'd like to free some names (...) event and many others should probably be inside Crystal::Runtime

👍

@ozra
Copy link
Contributor

ozra commented Sep 25, 2016

Woot?

the old foo finds foo/foo.cr doesn't apply anymore

Nooooo! I'd like that rule tried after foo.cr "everywhere"...

@asterite
Copy link
Member Author

@ozra @waj actually likes/d that rule too, so we could maybe put it back later, but only for the standard library (I don't know if it's useful for shards)

@RX14
Copy link
Contributor

RX14 commented Sep 26, 2016

I think that the rules should be kept as simple as possible, i.e. require "foo" shouldn't attempt to require foo/foo.cr. Personally, I would also remove the non-namespaced requires for the sake of simplicity and standardisation.

@ysbaddaden
Copy link
Contributor

@RX14 I'd rather have the namespaced rules removed instead, for reasons explained here and here.

@RX14
Copy link
Contributor

RX14 commented Sep 26, 2016

@ysbaddaden I guess it's a personal thing, but I really like the contents of src/ representing my namespaces.

I guess we should keep both styles then.

@asterite
Copy link
Member Author

asterite commented Oct 6, 2016

I think I'd like to put back the rule that require "foo" also finds foo/foo.cr, but only in the standard library. The std is pretty big and organizing things in directories will help it become a bit more clean. I might do that...

@asterite asterite force-pushed the feature/new_crystal_path branch 2 times, most recently from a584b3b to 4048ecb Compare October 6, 2016 13:46
@asterite
Copy link
Member Author

asterite commented Oct 6, 2016

UPDATE: I added one more general rule: for any require "foo/bar/baz", if a file can't be found with the rules above then if a directory named "baz" exists instead of a file (for any of the rules), and that directory has a "baz.cr" file, then that file will be required. This is only applied for the last component of a path.

With the above rule we get two things:

  1. The change is backwards compatible, and we don't need to synchronize the next release of crystal and shards. All existing code still compiles because doing require "foo" will find a file in libs/foo/foo.cr, which is how shards currently places things. Shards can be updated to check if libs exists: if so, remove it and checkout everything in lib (without just keeping the contents of the src directory). For one release I'll also change the default CRYSTAL_PATH to have both libs and lib in it.
  2. It lets the std and shards author better organize things.

The first point is the main reason I decided to do this change: backwards compatibility for at least one release, to more easily evolve shards and every code out there.

But the second point is also nice, and we could consider keeping it afterwards. It means that if someone does require "foo/bar", the std or shard could use this tree:

  • src
    • foo
      • bar
        • file1.cr
        • file2.cr
      • bar.cr

Or use this tree:

  • src
    • foo
      • bar
        • file1.cr
        • file2.cr
        • bar.cr

I personally find the second tree nicer, because everything related to bar is inside foo/bar.

@asterite asterite added this to the 0.19.4 milestone Oct 6, 2016
@asterite asterite merged commit fece2f7 into master Oct 6, 2016
@ozra
Copy link
Contributor

ozra commented Oct 6, 2016

👍 for everything related in the same dir :-)

If I have one file only for a class / functionality - I keep it directly accessible, without wrapping dir. But as soon as I split it up / add more files to the module - I move it down in its' own dir.
With above defined logic - that just keeps working :-)

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

Successfully merging this pull request may close these issues.

Improve require lookup Use lib instead of libs for require path
5 participants