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

Use local base64_ functions #423

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olleolleolle
Copy link
Contributor

@olleolleolle olleolleolle commented Feb 9, 2024

This PR completes the move away from the base64 gem.

  • First, I removed the development dependency on base64.
  • Then I typed all the changes to the test files.
  • Then, I added one more local implementation of the base64_ functions.

Then, two tests failed. I ran out of steam. Can you find what didn't add upp, @Earlopain? Update: BUT THEY ONLY FAILED LOCALLY?!

When this is done:

Fixes #422

@olleolleolle olleolleolle marked this pull request as ready for review February 9, 2024 17:40
@Earlopain
Copy link

I was writing a whole thing here but someone already went through the trouble in #417

@olleolleolle olleolleolle changed the title Attempt to make local base64_ functions Use local base64_ functions Feb 12, 2024
@santiagorodriguez96
Copy link
Contributor

Hi @olleolleolle! Good catch! Thank you for opening another PR for addressing this 🙂

Out of curiosity: is there a reason why you decided to remove the development dependency this time?

Regarding the failing tests:

@Earlopain got it right, the same is happening for me on local machine. If you have more info about the error that you consider might be relevant, please let us know on that issue 🙂

@olleolleolle
Copy link
Contributor Author

@santiagorodriguez96 Oh, I was just being thorough, one-track minded. It can of course be used as a development dependency.

Copy link
Contributor

@santiagorodriguez96 santiagorodriguez96 left a comment

Choose a reason for hiding this comment

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

Tbh I'm a little bit worried about having the implementations of base64 methods in three different places, but I think it's okay for the moment. I'll think about what can we do to prevent that duplication 🙂

Copy link
Member

@brauliomartinezlm brauliomartinezlm left a comment

Choose a reason for hiding this comment

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

hi all! Thank you so much @olleolleolle for doing this. Really appreciate the contribution ❤️

While I know we need to contemplate the removal of Base64 from Ruby's default I think this PR makes me reflect we need to encapsulate all the necessary functions we're adding in a module where we can keep it contained. I'm not very aligned with switching the usage of the dependency in favor of having all this logic spread out through the gem's code.

Would you be willing to evolve this PR to create a module that encapsulates it the logic and can be leveraged from everywhere in the gem?

I'm not gonna mark it "Request changes" (I don't love that feature's emotional communication 😄 ) but regardless of @santiagorodriguez96 approving I think we need to address that first.

Apologies for not jumping in earlier and giving this feedback after some back and forth

@olleolleolle
Copy link
Contributor Author

Absolutely, I'll poke at making a module, it'll happen in this PR.

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.

base64 cleanup needed: U2fMigrator still calls Base64 methods, and a require exists
4 participants