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

Improve performance of usable? helper #209

Closed
markets opened this issue Feb 7, 2023 · 5 comments · Fixed by #210
Closed

Improve performance of usable? helper #209

markets opened this issue Feb 7, 2023 · 5 comments · Fixed by #210

Comments

@markets
Copy link
Collaborator

markets commented Feb 7, 2023

All providers define this method:

def self.usable?(url)
  url ~= /PROVIDER_REGEX/
end

Since we don't need matches and it's defined as a predicate method, I think we should switch to String#match? or Regexp#match? in those methods for speed (https://github.com/fastruby/fast-ruby#stringmatch-vs-stringmatch-vs-stringstart_withstringend_with-code-start-code-end):

def self.usable?(url)
  url.match? /PROVIDER_REGEX/
end

What do you think @thibaudgg?

@thibaudgg
Copy link
Owner

Sure thing, please feel free to open a PR. 👍🏻

@markets
Copy link
Collaborator Author

markets commented Feb 8, 2023

@thibaudgg here you go 👉🏼 #210

(We should fix the build someday 🙏🏼, now it complicates contributing to this useful project... I'd take this task, but unfortunately I have no experience with VCR 😅)

@thibaudgg
Copy link
Owner

Yeah, the main issue is that VCR is configured to re-record the cassette (request mock) after a specified amount of time. Here it seems that most provider slightly changed their response so a lot of specs must be fixed. This is certainly a good opportunity to improve the specs base. It would be great if you can look at it, I can assist if you have some questions about VCR.

@markets
Copy link
Collaborator Author

markets commented Feb 9, 2023

Hey @thibaudgg 👋🏼 if you (or somebody that implemented this part) can fix 1 provider as an example, I'll finish the job! That would be super helpful 🤝

Thanks!

@thibaudgg
Copy link
Owner

What you could do is remove all the registered cassettes and fix the broken specs from there one by one. Sadly I will not have the time to look at it in the near future.

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 a pull request may close this issue.

2 participants