Try from err typos #229

brendanrbrown · 2021-07-18T13:51:00Z

Fixes minor typographical errors for the following conversions and adds corresponding integration tests:

TryFrom for Vec was Error::ExpectedInteger and is not Error::ExpectedReal
From<HashMap<String, Robj>> was missing, though &str impl existed

I did not fix the misspelling 'Enviroment' throughout extendr-api since it does not actually produce errors.

R documentation quashed for the conversion test functions.

clauswilke · 2021-07-18T15:52:26Z

Apologies for being picky. I think the hashmap code needs to be its own separate PR, and it probably needs some discussion first in an issue. It's not clear to me that we want to always convert a hashmap into a list, as opposed to an environment.

And for a PR that fixes spelling and other minor issues (such as @noRd tags) I think it should also fix the spelling of environment.

Finally, the integration tests are failing because R check notices that there are no man pages. I think @usage NULL will fix this. (But not entirely certain. I just know somehow it's possible to suppress documentation.)

brendanrbrown · 2021-07-18T17:20:39Z

Your comments make sense and aren't too picky. I agree discussing hashmap is reasonable. The lack of From HashMap<String, Robj> seemed to someone new on the project to be simply an oversight given existing From,TryFrom implementations for HashMap with &str and String keys.

edit: I should mention, too, that even if HashMap to List is deemed proper the existing implementation should handle better unnamed lists. Currently the conversion of an unnamed list produces an empty list, when I think probably it should just fail.

Will implement your suggestions and resubmit. Thanks

clauswilke · 2021-07-18T20:12:22Z

I've opened an issue about hasmaps: #230

brendanrbrown · 2021-07-20T01:06:05Z

recent upstream commits here, and

removal of HashMap<String, Robj> impl fix from previous push
Rd files suppressed with @usage NULL for all conversion test functions
Enviroment typo fixed throughout extendr-api
maintains fix for typo in TryFrom error type in double_vec

edit
A note on R CMD check: If we wanted to maintain other documentation elements in the source but still suppress Rd files, R CMD check shouldn't complain if we use @noRd and @keywords internal. I forgot the latter last go-round, but it has worked for me in other projects. I was not familiar with @usage NULL for this purpose, but it seemed only to suppress the Rd file entirely if there were no other documentation tags. Useful trick.

clauswilke · 2021-07-20T02:49:14Z

It's starting to look good, but there are some changes that don't seem to belong (the change in the changelog, the hashmap code). Overall, from experience, I'd say once you have more than a handful of commits chances are something went wrong and you'd be better off starting over. Also, never rebase, just merge in the main tree.

The tests fail because there's a formatting error. I'll flag it.

clauswilke · 2021-07-20T02:49:56Z

extendr-api/src/lib.rs

@@ -399,7 +399,7 @@ pub enum RType {
    Symbol,      // SYMSXP
    Pairlist,    // LISTSXP
    Function,    // CLOSXP
-    Enviroment,  // ENVSXP
+    Environment,  // ENVSXP


This line causes the tests to fail because the comment is not aligned.

clauswilke · 2021-07-20T02:50:07Z

CHANGELOG.md

@@ -22,6 +22,7 @@

 - Use Use failure() to trigger steps on failures

+- `SymPair::sym_pair()` now returns `(Option<Robj>, Robj)`


Not sure why this is here.

clauswilke · 2021-07-20T02:50:34Z

extendr-api/src/robj/into_robj.rs

+        let res: Robj = List::from_values(val.iter().map(|(_, v)| v)).into();
+        res.set_names(val.into_iter().map(|(k, _)| k)).unwrap()
+    }
+}


Not sure why this code block is here.

clauswilke · 2021-07-20T02:51:21Z

extendr-api/src/robj/try_from_robj.rs

+                    Err(Error::ExpectedString(robj))
+                }
+            }
+            _ => Err(Error::ExpectedScalar(robj)),


This is probably fine, but honestly it's mixing different things into one PR and I'd rather have a separate PR just for this change.

clauswilke · 2021-07-20T02:52:39Z

extendr-api/src/wrapper/environment.rs

-                unsafe { Rf_defineVar(n.get(), v.get(), robj.get()) }
+                if let Some(n) = n {
+                    unsafe { Rf_defineVar(n.get(), v.get(), robj.get()) }
+                }


I'm sure this is correct, but would you mind explaining? I don't see immediately how the previous code could compile and the new code also compiles.

clauswilke · 2021-07-20T02:54:05Z

extendr-api/src/wrapper/pairlist.rs

@@ -31,7 +37,7 @@ impl Pairlist {
                let val = Rf_protect(val.get());
                res = Rf_protect(Rf_cons(val, res));
                num_protects += 2;
-                if !name.is_unbound_value() {
+                if let Some(name) = name {


I think this is from a different PR.

clauswilke · 2021-07-20T02:54:19Z

extendr-api/src/wrapper/pairlist.rs

@@ -126,7 +132,8 @@ impl Iterator for PairlistIter {
                    let name = to_str(R_CHAR(printname) as *const u8);
                    Some((std::mem::transmute(name), value))
                } else {
-                    Some((na_str(), value))
+                    // empty string represents the absense of the name
+                    Some(("", value))


I think this is from a different PR.

clauswilke · 2021-07-20T02:54:31Z

extendr-api/src/wrapper/pairlist.rs

@@ -143,7 +150,7 @@ impl IntoIterator for Pairlist {
    /// test! {
    ///     let pairlist = pairlist!(a=1, 2).as_pairlist().unwrap();
    ///     let vec : Vec<_> = pairlist.into_iter().collect();
-    ///     assert_eq!(vec, vec![("a", r!(1)), (na_str(), r!(2))]);
+    ///     assert_eq!(vec, vec![("a", r!(1)), ("", r!(2))]);


I think this is from a different PR.

clauswilke · 2021-07-20T02:54:48Z

extendr-api/src/wrapper/symbol.rs

-                robj: unsafe { new_owned(make_symbol(val)) },
-            }
+        Symbol {
+            robj: unsafe { new_owned(make_symbol(val)) },


I think this is from a different PR.

clauswilke · 2021-07-20T02:58:25Z

tests/extendrtests/src/rust/src/lib.rs


-/// Return string `"Hello world!"` to R.
-/// @export
+/// @usage NULL


Not sure I like deleting the actual meaningful comments (such as "Return string ...). You probably don't need to add @usage NULL at all, and can instead use // rather than /// to indicate the comment. Then the comment doesn't get moved into the R wrapper, the function is not exported, and R check should be happy.

clauswilke · 2021-07-20T03:01:47Z

It's possible that a lot of things will be fixed if you just merge in master.

brendanrbrown · 2021-07-20T03:07:33Z

hmm, so i did not make most of the changes you commented on --- they come from the previous commits made yesterday (re: #226) and which i pulled this afternoon to avoid merge conflicts. the same is true from the below. **edit: ** last sentence makes no sense here anymore.

to your main point: it would have been cleaner to start over.

nonetheless, thank you for the time here, and please do let me know the preference in situations like this.

It's possible that a lot of things will be fixed if you just merge in master.

yes, will do

brendanrbrown · 2021-07-20T12:52:30Z

@clauswilke To clean up this git history mess a bit my plan is to merge to master on my fork and have the PR point there. I've not done that before as I typically am a maintainer on projects I work on. If doing so, with some loss of the commit history, is undesirable please do let me know. Trying to reduce the amount of nonsense I produce for review here going forward. Thanks again

clauswilke · 2021-07-20T13:28:09Z

Yes, all you have to do is merge master into your current branch and then push. The PR should update and GitHub should then only register the changes since master.

The history doesn't matter because when we merge the PR we squash. It's another reason why each PR should be one single issue only; it's going to be only one commit on the master branch.

andy-thomason

Looks good to me.

HashMap of Robj should convert without error.

We could make the String into a T : AsRef<str> to be more generic, but later will do.

clauswilke · 2021-07-21T13:04:56Z

I think this looks good now. Could you try to merge in master so we no longer see the changes that aren't actually part of your PR?

brendanrbrown · 2021-07-21T17:20:27Z

great, thanks for the comments all.

what follows is rather tedious. TLDR summary is that i can reopen a PR to reset history, switch the PR base branch and back to trick github, or do nothing. the PR merge shouldn't associate the unrelated commits to this PR, as the history displayed here is a cosmetic feature of github apparently.

on the commit history from upstream/master that isn't mine but appears here:

i did indeed merge my branch into origin/master (my fork master), but the issue of github showing commits from upstream/master appears to be a github issue. see this long-standing thread, still active:

https://stackoverflow.com/questions/16306012/github-pull-request-showing-commits-that-are-already-in-target-branch#26986320

annoying, though it seems this issue is mostly cosmetic since you will squash this PR's commits anyway.

i see a few options and am happy to do what is easiest or best from your perspective:

try one of the hacks listed above, such as switching PR base branch to something else and back. one person suggested this adds more history, so i did not just do it.
open a new PR and close this one, which should reset the history.
rebase to squash all commits into one and re-push. i am hesitant to do this as I am unsure whether this will make it appear as though i have done someone else's work. i don't believe so --- again, this commit history is a github display issue not a git issue ---- but am unsure.

clauswilke · 2021-07-21T18:36:45Z

I think we should try to get this right, most importantly so you've got the workflow down in the future, and also because if I merge now I'm quite certain it'll look like @yutannihilation co-authored the PR.

In brief, I think this is where you went wrong:

i did indeed merge my branch into origin/master (my fork master),

You need to merge upstream master into your branch, but you should not touch your fork master.

In general, the workflow that works is:

Make a fork of the project
Check out the fork and make a branch for development
Make commits, then push (the first time with git push --set-upstream origin <branch>)
Then make a PR
If changes are needed, make more commits and push
If the PR can't be merged because of a conflict, fetch upstream, merge upstream/master into your branch, fix conflicts, and push
Repeat 5 and 6 until ready for merge

brendanrbrown · 2021-07-21T19:48:39Z

looks like a new PR is the cleanest solution i think. if you would prefer, i could squash the history into one commit, with me as author.

In brief, I think this is where you went wrong:

in case this is a reference to my future self or others: i should have been more clear. i did that only after the previous commit history from origin/master had shown up in this PR, so it cannot be the cause, at least not this time.

but your comment is still valid, i think, in broad terms: these errant commits, which were updates to origin/master while this PR was ongoing, showed up after i merged upstream/master into origin/master then into my branch, rather than merging upstream/master directly into my branch. otherwise, i followed your steps 1-7. step 6 was the key here, in other words. noted.

clauswilke · 2021-07-21T21:05:03Z

looks like a new PR is the cleanest solution i think. if you would prefer, i could squash the history into one commit, with me as author.

Whichever is easier for you. I don't particularly care. We squash when we merge into master.

* fix TryFrom<Robj> for Vec<f64> Err type * add associated tests in extendrtests, plus one for HashMap<str, Robj>

brendanrbrown · 2021-07-22T01:31:51Z

ok, this should fix things. thanks for the back and forth.

yutannihilation · 2021-07-22T01:48:57Z

CI failures on R-devel runner are probably irrelevant to this pull request, so please ignore (probably due to the recent API change on R-devel, which itself is a bit worrisome...).

  error[E0425]: cannot find function, tuple struct or tuple variant `SET_PRSEEN` in this scope
      --> extendr-api/src/wrapper/promise.rs:27:13
       |
  27   |             SET_PRSEEN(sexp, 0);
       |             ^^^^^^^^^^ help: a function with a similar name exists: `SET_PRENV`
       | 
      ::: /home/runner/work/extendr/extendr/target/debug/build/libR-sys-fe61ba3184160aea/out/bindings.rs:4969:5
       |
  4969 |     pub fn SET_PRENV(x: SEXP, v: SEXP);
       |     ----------------------------------- similarly named function `SET_PRENV` defined here

clauswilke · 2021-07-22T04:31:00Z

I'm still confused by the hashmap situation but I'm going to merge since this contains many useful fixes. We can sort out the hashmap in future PRs.

clauswilke mentioned this pull request Jul 18, 2021

Conversions to and from hashmap #230

Closed

clauswilke reviewed Jul 20, 2021

View reviewed changes

andy-thomason approved these changes Jul 21, 2021

View reviewed changes

* fix 'enviroment' typos throughout

a611f97

* fix TryFrom<Robj> for Vec<f64> Err type * add associated tests in extendrtests, plus one for HashMap<str, Robj>

clauswilke merged commit a889113 into extendr:master Jul 22, 2021

clauswilke mentioned this pull request Jul 22, 2021

Figure out what to do about R-devel API changes #235

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try from err typos #229

Try from err typos #229

brendanrbrown commented Jul 18, 2021 •

edited

Loading

clauswilke commented Jul 18, 2021

brendanrbrown commented Jul 18, 2021 •

edited

Loading

clauswilke commented Jul 18, 2021

brendanrbrown commented Jul 20, 2021 •

edited

Loading

clauswilke commented Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke Jul 20, 2021

clauswilke commented Jul 20, 2021

brendanrbrown commented Jul 20, 2021 •

edited

Loading

brendanrbrown commented Jul 20, 2021

clauswilke commented Jul 20, 2021

andy-thomason left a comment

clauswilke commented Jul 21, 2021

brendanrbrown commented Jul 21, 2021

clauswilke commented Jul 21, 2021

brendanrbrown commented Jul 21, 2021

clauswilke commented Jul 21, 2021

brendanrbrown commented Jul 22, 2021

yutannihilation commented Jul 22, 2021

clauswilke commented Jul 22, 2021

		@@ -22,6 +22,7 @@

		- Use Use failure() to trigger steps on failures

		- `SymPair::sym_pair()` now returns `(Option<Robj>, Robj)`

Try from err typos #229

Try from err typos #229

Conversation

brendanrbrown commented Jul 18, 2021 • edited Loading

clauswilke commented Jul 18, 2021

brendanrbrown commented Jul 18, 2021 • edited Loading

clauswilke commented Jul 18, 2021

brendanrbrown commented Jul 20, 2021 • edited Loading

clauswilke commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clauswilke commented Jul 20, 2021

brendanrbrown commented Jul 20, 2021 • edited Loading

brendanrbrown commented Jul 20, 2021

clauswilke commented Jul 20, 2021

andy-thomason left a comment

Choose a reason for hiding this comment

clauswilke commented Jul 21, 2021

brendanrbrown commented Jul 21, 2021

clauswilke commented Jul 21, 2021

brendanrbrown commented Jul 21, 2021

clauswilke commented Jul 21, 2021

brendanrbrown commented Jul 22, 2021

yutannihilation commented Jul 22, 2021

clauswilke commented Jul 22, 2021

brendanrbrown commented Jul 18, 2021 •

edited

Loading

brendanrbrown commented Jul 18, 2021 •

edited

Loading

brendanrbrown commented Jul 20, 2021 •

edited

Loading

brendanrbrown commented Jul 20, 2021 •

edited

Loading