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

case-insensitive gem search #107

Merged
merged 1 commit into from
Nov 21, 2024
Merged

case-insensitive gem search #107

merged 1 commit into from
Nov 21, 2024

Conversation

trusche
Copy link
Contributor

@trusche trusche commented Nov 20, 2024

Fixes #106. Review with "hide whitespace" please. Love the app, wasn't aware of it before your LinkedIn post. Thanks!

@@ -21,3 +21,6 @@ storage
tmp

public/sitemap.xml.gz

app/assets/builds
public/assets
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were generated locally but not ignored. I assume they should be.

4. Go to http://localhost:3000
3. `rake data:find_or_create_rails_releases`
4. `foreman start -f Procfile.dev`
5. Go to http://localhost:3000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed this extra step to get the app up and running

@@ -1,40 +1,42 @@
require "fog-aws"
unless Rails.env.local?
Copy link
Contributor Author

@trusche trusche Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit brute force, but loading this caused errors locally that can be ignored, I think, if we don't want to connect to AWS in development. I can take it out if you like, but this prevents local dev from running without AWS keys. The only change is the guard clause, the rest is whitespace.

@@ -6,7 +6,7 @@ def create
if name == "rails"
RailsReleases::Create.perform_async params.fetch(:version)
else
Gemmy.find_by(name: name)&.then {
Gemmy.find_by_name(name)&.then {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a global search and replace for find_by!?(name:...


def self.find_by_name!(name)
find_by_name(name, raise_error: true)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and added these two methods for case-insensitive search.

The other option of course would be to change the data type to citext, which requires a postgres extension. I think that would be cleaner but didn't want to go quite that far.

Copy link
Member

@etagwerker etagwerker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trusche Looks good, thank you! 💯

@etagwerker etagwerker merged commit f9f8cee into railsbump:main Nov 21, 2024
1 check passed
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.

Gem search should default to lower case naming convention
2 participants