D1581: rust: implementation of `hg`

[kevincox (Kevin Cox)]

kevincox added a comment.


  Overall it looks great. I added a bunch of nitpicks. The most common suggestion was to add some contextual information to error messages. Other then that mostly minor style improvements. Feel free to push back on anything you don't agree with :)

INLINE COMMENTS

> cramertj wrote in build.rs:88
> Nit: not sure what version you're targeting, but `'static` is automatic for `const` vars, so you could write `[&str; 2]`

I would also recommend using a slice if you don't intend the size of the array to be part of the type signature.

  const REQUIRED_CONFIG_FLAGS: &[&str] = &["Py_USING_UNICODE", "WITH_THREAD"];

> build.rs:33
> +        );
> +    }
> +

assert!(
      Path::new(&python).exists(),
      "Python interpreter {} does not exist; this should never happen",
      python);

I would also recommend `{:?}` as the quotes highlight the injected variable nicely and it will make some hard-to-notice things more visible due to escaping.

> build.rs:110
> +        if !result {
> +            panic!("Detected Python requires feature {}", key);
> +        }

Use `assert!`.

> build.rs:116
> +    if !have_shared(&config) {
> +        panic!("Detected Python lacks a shared library, which is required");
> +    }

Use `assert!`

> build.rs:127
> +        panic!("Detected Python doesn't support UCS-4 code points");
> +    }
> +}

#[cfg(not(target_os = "windows"))]
  assert_eq!(
      config.config.get("Py_UNICODE_SIZE"), Some("4"),
       "Detected Python doesn't support UCS-4 code points");

> main.rs:37
> +fn get_environment() -> Environment {
> +    let exe = env::current_exe().unwrap();
> +

Use expect for a better error message. `.expect("Error getting executable path")`

> main.rs:91
> +        .unwrap()
> +        .into_raw();
> +    unsafe {

This method allows paths that aren't valid UTF-8 by avoiding ever becoming a `str`.

  CString::new(env.python_home.as_ref().as_bytes())

I would also change the unwrap to `.expect("Error setting python home")`.

> main.rs:99
> +    // Call sys.setdefaultencoding("undefined") if HGUNICODEPEDANTRY is set.
> +    let pedantry = env::var("HGUNICODEPEDANTRY").is_ok();
> +

It appears that HG accepts `HGUNICODEPEDANTRY=` as not enabling unicode pedantry. Maybe the behavior should be the same here. Untested code below should work.

  let pedantry = !env::var("HGUNICODEPEDANTRY").unwrap_or("").is_empty();

> main.rs:111
> +fn update_modules_path(env: &Environment, py: Python, sys_mod: &PyModule) {
> +    let sys_path = sys_mod.get(py, "path").unwrap();
> +    sys_path

`.expect("Error accessing sys.path")`

> main.rs:133
> +    // not desirable.
> +    let program_name = CString::new(env.python_exe.to_str().unwrap())
> +        .unwrap()

`CString::new(env.python_exe.as_ref().as_bytes())`

> main.rs:134
> +    let program_name = CString::new(env.python_exe.to_str().unwrap())
> +        .unwrap()
> +        .as_ptr();

`.expect("Error setting program name")`

> main.rs:200
> +fn run_py(env: &Environment, py: Python) -> PyResult<()> {
> +    let sys_mod = py.import("sys").unwrap();
> +

Since this method returns a Result why not handle the error?

  let sys_mod = py.import("sys")?;

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1581

To: indygreg, #hg-reviewers, yuja, durin42
Cc: kevincox, cramertj, yuja, quark, durin42, dlax, mercurial-devel