Skip to content

Exact match not being obvious #493

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

Closed
Keats opened this issue Dec 13, 2016 · 11 comments
Closed

Exact match not being obvious #493

Keats opened this issue Dec 13, 2016 · 11 comments
Labels
A-frontend 🐹 A-search C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works E-has-mentor E-help-wanted

Comments

@Keats
Copy link

Keats commented Dec 13, 2016

When doing a search, a crate matching exactly the search will appear first, even if sorted by downloads. I initially thought it was a bug: #396 (comment)

Would it be possible to extract the exact match out of the list and display it differently?

@carols10cents carols10cents added A-search A-ui C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works labels Dec 15, 2016
@carols10cents
Copy link
Member

carols10cents commented Feb 11, 2017

Changing this bug's level to medium, because it involves a bunch of little pieces scattered all over. I think each of the pieces individually should be easy, but putting them all together might take a little while.

The pieces:

select id, name, name = 'sassers' as exact_match from crates;
 id |      name       | exact_match
----+-----------------+-------------
  2 | sassers         | t
  6 | piston          | f
  • Rust: Adding an exact_match: bool field to the EncodableCrate struct to hold the value from that select query and send it back in the API results. Note that all the other places that EncodableCrate is used should just get false for this field... not sure if adding exact_match: false or implementing the Default trait on EncodableCrate will be more annoying :)

  • Ember: Adding the exact_match property to the Ember crate model

  • Handlebars: Adding a class to this element if crate.exact_match is true

  • CSS: Styling that class to visually differentiate the exact match crate somehow :)

If anyone wants to pick off a piece of this, I'd accept PRs for each little bit separately-- I don't think any of these individually should break other things (famous last words ;) )

@venkatagiri
Copy link
Contributor

@carols10cents I wanted to work on this and ran cargo test before making any changes and few tests are failing trying to upload to S3 bucket. Logs are here http://pastebin.com/Hrei2g7H
README says "No actual requests to s3 will be made". Not sure what's wrong with my setup.

@carols10cents
Copy link
Member

Hi @venkatagiri, there is something weird with the .env file and the way crates.io uses env vars that I was hoping to fix before I needed to document it but I haven't yet :( Could you try changing your .env file to have export S3_REGION= instead of whatever you have for that variable?

If you already have that, could you pastebin your .env file, without any usernames, passwords, or keys of course?

Sorry about this! It's totally not your fault.

@venkatagiri
Copy link
Contributor

The S3_REGION variable is empty. Here's the .env file I am using. http://pastebin.com/gLDh31Dn
Just FYI, I have PostgreSQL running in a docker container.

If you can point me to the piece of code where the S3 calls get mocked, I will take a go at debugging this and fix it.

@carols10cents
Copy link
Member

Hmm, strange. So in tests, the Config that gets used is defined here, getting S3_REGION from the environment:

s3_region: env::var("S3_REGION").ok(),

The s3_proxy set in the config in the tests comes from record::proxy() The src/tests/record.rs file implements the mocking-- if the tests are run in recording mode, it stores a file of request/response data per test in src/tests/http-data. Then in replay mode (most of the time), when the config gets a request to s3, it tries to play back the data from the file for that test. The error you're seeing comes from this spot in record.rs that validates the request headers, but for some reason your app is creating requests to http://alexcrichton-test.s3-.amazonaws.com and the http-data files have requests to http://alexcrichton-test.s3.amazonaws.com recorded, so the headers don't match up.

This is where the S3 host gets constructed, so the http-data expect S3_REGION to be None but for some reason when you run the tests S3_REGION is Some(""), is what I think the problem is.

Something to try might be removing S3_REGION from your .env file completely, but I'd love to see a more robust solution to this problem. This is kind of related to #470... for some reason that I haven't completely figured out how to change yet, I have to set S3_REGION="" for running the server locally and S3_REGION= when running the tests :(

@venkatagiri
Copy link
Contributor

venkatagiri commented Feb 25, 2017

Yes. It seems to return Some("") for S3_REGION.
Modifying it with below changes fixes the issue with cargo test.

Is this change fine enough for a pull request or is there a simpler way to fix it?

vagrant@jessie crates.io $ gd
diff --git a/src/s3/lib.rs b/src/s3/lib.rs
index f4a43af..aa7d824 100644
--- a/src/s3/lib.rs
+++ b/src/s3/lib.rs
@@ -99,7 +99,8 @@ impl Bucket {
     pub fn host(&self) -> String {
         format!("{}.s3{}.amazonaws.com", self.name,
                 match self.region {
-                    Some(ref r) => format!("-{}", r),
+                    Some(ref r) if r != "" => format!("-{}", r),
+                    Some(_) => String::new(),
                     None => String::new(),
                 })
     }

@carols10cents
Copy link
Member

That looks great to me! I might end up rearranging things if I figure out what I want to do about #470, but this looks like it would fix the problem for now. Please send this change in a PR and I'll merge! Thank you for your patience!! ❤️

@carols10cents
Copy link
Member

Hi @venkatagiri, have you made any progress on this? I don't want to steal it from you if you have, but I'm thinking about using this bug at an OSS hack fest at a local college on Saturday, April 8, since it splits up into pieces nicely.

If you have any part that you'd like to submit, I'd love PRs for small pieces! Please let me know if you have any pieces by Saturday, so that I'm not duplicating any of your work.

If you haven't had a chance to make progress on this, no worries :) I'm happy to help you find something else whenever you have time! ❤️

@venkatagiri
Copy link
Contributor

venkatagiri commented Apr 3, 2017

@carols10cents, I didn't get a chance to work on this. So, go ahead and use this at the hack fest.

@carols10cents
Copy link
Member

To get some crates into your database to be able to test, start a psql session with:

psql

Then connect to the database crates.io uses with:

\c cargo_registry

Then insert some crates, named whatever you want. This will insert a crate named "hello":

insert into crates (name) values ('hello');

Then quit postgres with:

\q

@carols10cents
Copy link
Member

Fixed by #673, #674, and #675 by students at the CyberWVU FOSSFest 2017!!!!! 🥇 🥇 🥇 🥇 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend 🐹 A-search C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works E-has-mentor E-help-wanted
Projects
None yet
Development

No branches or pull requests

4 participants