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

Exposing the EXPORT_MODE_* constants and an optional "mode" argument to the export method #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gabrielfalcao
Copy link

Hey thanks for maintaining this binding to gpgme.
I wanted to export secret keys, so I exposed the necessary library members to the Python API.

Please let me know what you would like me to improve in this PR in order to get it merged to the HEAD.

One question: how did you manage to get the test suite running without need for human interaction (i.e: bypass pinentry entirely) ?
I asked that because there is a follow-up patch for the test suite as well, but need to get repeatable green builds before that can be worked on.
FYI2: I'm willing to do a major refactoring in the "unit" tests and reach 100% of test coverage.If that sounds good let's talk more about it :)

Cheers!

@rshk
Copy link
Owner

rshk commented Feb 17, 2017

Looks good to me, from a quick look. I haven't been working on this in a while, so I don't remember how I used to run the test suite, I guess there has to be some way to bypass pinentry though.

Why did you rename to xgpg by the way? Because of the name conflict on pypi? Be careful with automated replace btw, in some places you actually want to keep the "gpgme" name (eg docs saying "wrapper for gpgme library"..).

About improving tests, that sounds great; I think the ideal thing is to make it run on circleci / travis as well, if possible.

Thanks

@gabrielfalcao
Copy link
Author

Oh thanks for the quick response. The reason I had named xgpg was due
to the assumption that you would not reply as fast as you
did. (Renaming the namespaces in the source code took about 30
minutes, thanks to its wide-ranged test suite)

Anyways, there is no reason to rename it to xgpg now, so I'm renaming
back.

Nevertheless I would still like to propose and implement a few to the
codebase:

  1. Refactor the test codebase:

    • Get rid of test_all.py and
      use nose as a test
      runner instead.

    • Move current tests under the folder tests/functional/, since
      they perform real disk I/O.

    • replace the current fixture keys with password-less keys in
      order to prevent pinentry alert issues.

    • final goal: get a green build

  2. Get this PR merged

  3. Create a new PR introducing more improvements to the test codebase:

    • use sure for idiomatic
      assertions and modular scenario (setup/teardown) definitions. The
      idea is to make the tests an educational environment for future
      contributors.
    • Write unit tests that will live under tests/unit
    • Test-driven design a high-level "Keyring" python API that
      automates all the menial/repetitive tasks of the cookbook (i.e:
      setting a custom GPG directory, sign/verify/encrypt/decrypt
      file-like objects and strings, export armored keys, etc.
  4. Get to 100% of unit and functional test coverage, automated
    commands for setting up the development environment, run tests and
    build the documentation.

What are your thoughts on those points, I am hoping we can at least get to the 2 👍

I can get this all done pretty quickly, I'm already at the edge of my chair.

Cheers 🍺

@gabrielfalcao
Copy link
Author

Also I solely promise to honor this oath ;)

@gabrielfalcao
Copy link
Author

Hey @rshk, I hope you're doing well.
Would you let me know are are your thoughts on this when you find some time?
Cheers!

@rshk
Copy link
Owner

rshk commented Mar 26, 2018

Hi @gabrielfalcao, just found this seemingly-forgottend PR.. sorry about that.
Looks good overall so I guess we can get it merged. Have you done any work on the testing thing you mentioned? We can have a separate discussion about that

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