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

Parameterised Secret and made the number of stars used not the same length as the underlying value #4

Closed
wants to merge 66 commits into from

Conversation

Michaelt293
Copy link
Contributor

No description provided.

@afsalthaj
Copy link
Owner

afsalthaj commented Dec 26, 2018

@Michaelt293 Awesome one 👍
Thanks for a direct PR all the way

@afsalthaj
Copy link
Owner

afsalthaj commented Dec 26, 2018

@Michaelt293 Would u mind resolving the conflicts? Sorry that I kept pushing things after that.. That is mainly on automatic Safe instances using another set of macros

@Michaelt293
Copy link
Contributor Author

Michaelt293 commented Dec 26, 2018

Sure, I don't mind resolving merge conflicts.

Now I have thought about it a bit longer, I am not sure whether we should have hiddenLength as a method on the Safe type class. Perhaps it would be easier just to hide the value with a masking string of a constant ~six stars. That might make the macro you have easier to implement and I'm not sure how much value we get from the length of the hidden value (masking string) being dependent on the type. Also, I strongly believe that we should parameterise Secret so that we can hide types other than string (as done in this PR). If we make the masking string a constant six stars, we no longer need a Safe type class constraint on the Safe type class instance for Secret and will be able to hide any type.

@afsalthaj
Copy link
Owner

afsalthaj commented Dec 27, 2018 via email

@afsalthaj
Copy link
Owner

Also very keen to get your PR (the parameterisation of secret and implementation of your suggestion of removal of length of password) resolved with conflicts.

@Michaelt293
Copy link
Contributor Author

I've resolved the merge conflict now. I think there might be an issue with the SBT build since I was getting a build error when I tried running the test.

@Michaelt293
Copy link
Contributor Author

Also, I can rebase my branch if you want tidy commits.

@afsalthaj
Copy link
Owner

afsalthaj commented Dec 27, 2018

@Michaelt293 I would like you to push this PR again with the suggestions mentioned.The PR is closed automatically as I did a commit history cleaning. Sorry for the incovenience but I had to do this to get rid of the weird commit history in master branch and I had to clean it before the project gets more audience, otherwise it can be problematic in 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 this pull request may close these issues.

None yet

2 participants