Patching wincred backend to include encoding options

[awong234]

Addresses #85 .

Setter updates

• Updates b_wincred_set_with_value, encoding the password with an encoding if and only if a) and option is set or b) and environment variable is set.
  • The implementation is https://github.com/awong234/keyring/blob/7d4abd97b4afc824cf5eac0e41f7662ac0e04366/R/backend-wincred.R#L306-L312
  • I did not want to modify the keyring API, and thus did not provide any additional arguments for encoding. This could be easily altered though, but all backends would have to accept this new argument.
• Adds separate python and encoding tests
  • Adds reticulate to suggests for tests
  • Conditionally run python test only if conda is set up properly, otherwise skip
• Updates help docs for encoding as it relates to setter functions and Windows. Notes encodings that don't work (problem specific to iconv itself).

Getter updates

• Encoding is obtained using get_encoding_opt() in R/utils.R. This will: a) Check options and environment settings for keyring.encoding.windows and KEYRING_ENCODING_WINDOWS, respectively. b) Stop if both are set and not equal. c) If one is set, pick whichever encoding is not 'auto'. If none are set, then it is 'auto' d) Check against iconvlist, offering a suggestion in the case of typos. e) Return the encoding value.
• Adds two new wincred functions; b_wincred_decode, and b_wincred_decode_auto. The latter is just previous established behavior converting raw byte strings under UTF-8 or UTF-16LE encoding, as in https://github.com/r-lib/keyring/blob/da3e1796f606d9ce3a735a40efcef88284bada30/R/backend-wincred.R#L239-L249 verbatim. The former uses either b_wincred_decode_auto, or if an encoding is set in the environment/options, uses that encoding to decode raw passwords.
• Adds tests.
• Adds conda environments for testing against python.

All tests pass.

I consider this to be draft status; while all tests pass and the intended functionality of getting and setting encodings in 92% of all values in iconvlist() works (python included), a few outstanding issues/questions remain:

• Should this be implemented in other backends? I don't have the capacity to test MacOS, although I could potentially fix for Linux.
  • As it stands currently, if we do implement in other backends, the option name should be reduced to keyring.encoding instead of the current keyring.encoding.windows.
• Should the API change, and by how much?
  • Should an encoding argument be added to the get/set functions? This would require either implementing this in all other backends, or notifying on macos and linux that only windows is supported at this time.
  • Should an additional method be added instead, perhaps set_with_encoding?
  • How should messaging be handled? I would like to do something like the lifecycle package, where messages are shown once per day, but their implementation requires rlang and I did not want to add dependencies unnecessarily, but in the case of 'auto' functionality I also don't think that constant messaging to set options is warranted in most use cases.

Thank you for your review!

[gaborcsardi]

Thanks! This is a big PR, so I'll get to it near the next keyring release. Just one quick question now:

Updates b_wincred_set_with_value, encoding the password with an encoding if and only if a) and option is set or b) and environment variable is set.

Isn't this a breaking change?

[awong234]

Hi Gabor, thank you for taking a look. I think that it wouldn't be a breaking change (although certainly correct me if I am mistaken) because the API was not changed, and the user experience was kept the same so old scripts will not break. My statement that you quoted is misleading, I apologize; if nothing is explicitly done by the user (by setting an option, or environment variable), then the behavior of keyring is the same as before.

Details

If a user desires a specific encoding it is obtained via a new function get_encoding_opt, which

• resolves to 'auto' if none are set
• prefers the non-'auto' option if both the R option or R environment variable are set, where one is 'auto' and does not match the other, and
• returns an error if the R option and R environ are both set, neither are 'auto', and specify different values.

This is then passed to b_wincred_decode, which is passed the encoding obtained from get_encoding_opt, and either uses b_wincred_decode_auto (which just wraps around existing encoding behavior), or encodes with the users' specified encoding.

[codecov-commenter]

Codecov Report

Merging #88 (c1c5fbd) into master (c91e87d) will decrease coverage by 0.58%. The diff coverage is 0.00%.

:exclamation: Current head c1c5fbd differs from pull request most recent head e570f8f. Consider uploading reports for the commit e570f8f to get more accurate results

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
- Coverage   32.78%   32.19%   -0.59%     
==========================================
  Files          15       15              
  Lines        1269     1292      +23     
==========================================
  Hits          416      416              
- Misses        853      876      +23     

Impacted Files	Coverage Δ
R/api.R	35.29% <ø> (ø)
R/backend-wincred.R	0.00% <0.00%> (ø)
R/utils.R	25.00% <0.00%> (-10.49%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c91e87d...e570f8f. Read the comment docs.

[gaborcsardi]

Thanks, merged now!

[awong234]

Just reviewed the commits, thanks for the review & merge Gabor! Glad to have helped.