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

Added a builder method to sign/encrypt as a text document #61

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

Conversation

bjansen
Copy link

@bjansen bjansen commented Mar 22, 2021

#60

@bjansen
Copy link
Author

bjansen commented Mar 22, 2021

I'm not sure how to modify the builder without causing compilation errors in existing projects that are not interested in this new feature. I added a method to Build, but this means it could theoretically be called multiple times:

BouncyGPG
	.encryptToStream()
	.withConfig(...)
	.withStrongAlgorithms()
	.toRecipients(...)
	.andSignWith(...)
	.binaryOutput()
	.textMode()
	.textMode()
	.textMode()
	.textMode()
	.andWriteTo(...)

Also, I haven't added any unit tests yet.

@bjansen
Copy link
Author

bjansen commented Apr 14, 2021

@neuhalje were you able to take a look at this PR by any chance?

@neuhalje
Copy link
Owner

Sorry, I completely missed that one!

To remove the "multiple calls possible " topic: Add a new interface that derives from Build and returns Build. Return that from the previous step.

The example projects are a good way to test building: just update their grade/Pom and run the shell script.

Could you also add some unit test?

My next weekends are already planned - I can do an in depth review in two weeks

@bjansen
Copy link
Author

bjansen commented May 4, 2021

Thanks for the suggestions! I added a unit test which checks that encrypted data can be decrypted, but I'm not sure how to check the data is actually encoded as text literal packets. I guess I could use BouncyCastle directly, but I'm not proficient enough with this library.

@bjansen
Copy link
Author

bjansen commented Jun 22, 2021

@neuhalje do you think you'll have some time in the near future to review this PR? Thanks

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.

2 participants