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

Implement All Your Bases #170

Closed
wants to merge 9 commits into from
Closed

Implement All Your Bases #170

wants to merge 9 commits into from

Conversation

jonasbb
Copy link
Contributor

@jonasbb jonasbb commented Jul 12, 2016

Problem description and test data from x-common repository.

Design decisions:

  • Empty list is equal to 0
  • No leading 0s
  • Invalid input must result in Err(.) but not specified which one

Problem Description
Test data

This exercise should be the replacement (1) for all the individual base conversion exercises like Binary or Octal. Those should probably be marked in the config.json as not to implement.

Can we add an individual Markdown which describes these design decisions as well as the data type for the digits? Digits are just i64 (or any other number type) instead of something like char because there is a test with base 97.

Problem description and test data from x-common repository.

Design decisions:
 * Empty list is equal to 0
 * No leading 0s
 * Invalid input must result in Err(.) but not specified which one
@IanWhitney
Copy link
Contributor

Can we add an individual Markdown which describes these design decisions as well as the data type for the digits? Digits are just i64 (or any other number type) instead of something like char because there is a test with base 97.

You could put that in a markdown file, or you could put it in the stub src/lib.rs file.

@jonasbb
Copy link
Contributor Author

jonasbb commented Jul 13, 2016

Sure, that's what I want to do. Does the markdown file need a special name?


#[test]
#[ignore]
fn first_base_is_zero() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that these test names come from the canonical set, but I think "first" should be replaced with "input", and "second" should be replaced with "output", since that reflects the language used in the tests.

@IanWhitney
Copy link
Contributor

Sure, that's what I want to do. Does the markdown file need a special name?

I don't think so. Looking at the exercises that have extra files (Tournament, Hello World), they naming should not matter.

@IanWhitney
Copy link
Contributor

Thoughts on where we place this in the track?

@jonasbb
Copy link
Contributor Author

jonasbb commented Jul 15, 2016

As I understood the intention behind this exercise it should replace all existing conversion exercises. The Rust track already has the hexadecimal exercise. I think it should deprecate hexadecimal and replace it with all your base. Looking at the sample solutions, all your base is a bit more complex, mostly because both bases are now variables. However, you need the same tools to solve it.

Topics (as mentioned in your pull request)

Hexadecimal All your bases
Option Result
zip (but can also use enumerate) enumerate (but can also use zip)
fold fold
chars (not needed)
map map

I will fix the placement of hexadecimal in my pull request.

@jonasbb jonasbb changed the title Implement All Your Base Implement All Your Bases Jul 15, 2016
@petertseng
Copy link
Member

petertseng commented Jul 17, 2016

is exercism going to get confused by the fact that this here is called all-your-bases, but in x-common it is called all-your-base? I think I'm OK with the exercise content

@petertseng
Copy link
Member

Philosophically speaking... if the function is expected to treat negative bases as invalid, why should it still take i32 rather than u32? What if we let type system ensure for us that we never have a negative base?

(Other than the fact that the x-common test cases have a negative base. We can just ignore those tests cases if we use unsigned inputs)

There are times when I chose to use a signed integer and that was because even though any individual value would never go negative, sometimes I subtract two values and that might go negative. There may be similar reasons here.

@jonasbb
Copy link
Contributor Author

jonasbb commented Jul 17, 2016

@petertseng Thanks for the note about the typo. I didn't intent to change the name. I shouldn't do this late at night.

I don't know if it is better to use signed or unsigned integers. I struggled with the decision while writing the tests, but then decided it would be better to keep them as close as possible to the shared test data.

Looking at my sample solution I find only one half of the checks could be removed, namely num < 0 || while testing if all digits are in the valid range. The test for the bases would be unchanged. A solution would be to change the mappings of both bases so that the value 0 would correspond to base 2, because it is the first valid base. But this might be unnecessary complex, but being safe.

@petertseng
Copy link
Member

One more small thing to fix: the test file is still called all-your-bases.rs

It's true that any solution would still have to check the bases. It however seems too unclear to subtract two from every base, as it would be confusing. So even if this exercise moved to use unigned integers, I wouldn't touch the values.

I don't feel strongly enough to say it must be either one way or the other, so I leave that to you and others who might feel strongly.

@jonasbb
Copy link
Contributor Author

jonasbb commented Jul 18, 2016

It is probably unnecessary to have signed numbers so I changed it. I updated all the description texts and renamed the test file.

@@ -0,0 +1,177 @@
extern crate allyourbases as ayb;
Copy link
Member

@petertseng petertseng Jul 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last allyourbases here

@petertseng
Copy link
Member

OK, if you have decided on unsigned that seems reasonable, I'm sorry I missed a few lingering bases, after that we should be good to go 👍

@IanWhitney
Copy link
Contributor

Looks like the one thing preventing a merge are the three remaining "bases" that @petertseng noted. Once those are fixed, we'll merge.

@petertseng petertseng mentioned this pull request Aug 24, 2016
4 tasks
@petertseng
Copy link
Member

It has been a month, I have merged this as https://github.com/petertseng/xrust/commit/0c10aa3f5b062d2aae4cc1e396281968a8fa70ee - thank you @jonasbb for the contribution.

@petertseng petertseng closed this Sep 5, 2016
@petertseng
Copy link
Member

Oh wait, hold on, need to put that in the right repo.

@petertseng petertseng reopened this Sep 5, 2016
@petertseng
Copy link
Member

Merged as 9c527e4, thank you!

@petertseng petertseng closed this Sep 5, 2016
@petertseng
Copy link
Member

To examine changes I made, run:

diff -u <(git diff 6684722ec42e22ca67b10ef93891a0a9728991b8~9 6684722ec42e22ca67b10ef93891a0a9728991b8) <(git diff 9c527e465fd95eef5b4243f123acec76892f212f~ 9c527e465fd95eef5b4243f123acec76892f212f)

@petertseng
Copy link
Member

petertseng commented Sep 5, 2016

So that more are able to see the changes I made,

I will paste the output of that command for me since it might not be the case that everyone has 6684722. Note that this is a diff-of-diff, so you mostly care about the lines prefixed with two symbols either plus or minus.

The summary is that I changed the remaining "bases" to "base" and replaced hexadecimal with all-your-base in problems.md

$ diff -u <(git diff 6684722ec42e22ca67b10ef93891a0a9728991b8~9 6684722ec42e22ca67b10ef93891a0a9728991b8) <(git diff 9c527e465fd95eef5b4243f123acec76892f212f~ 9c527e465fd95eef5b4243f123acec76892f212f)
--- /proc/self/fd/11    2016-09-05 00:24:21.607889398 -0700
+++ /proc/self/fd/12    2016-09-05 00:24:21.607889398 -0700
@@ -1,5 +1,5 @@
 diff --git a/config.json b/config.json
-index 328573d..cee4327 100644
+index 0775fa7..9ca5b8c 100644
 --- a/config.json
 +++ b/config.json
 @@ -20,7 +20,7 @@
@@ -11,7 +11,7 @@
      "grade-school",
      "tournament",
      "robot-simulator",
-@@ -41,7 +41,7 @@
+@@ -45,7 +45,7 @@
      "circular-buffer"
    ],
    "deprecated": [
@@ -20,7 +20,7 @@
    ],
    "ignored": [
      "_test",
-@@ -49,6 +49,8 @@
+@@ -53,6 +53,8 @@
      "img"
    ],
    "foregone": [
@@ -32,22 +32,22 @@
  }
 diff --git a/exercises/all-your-base/Cargo.lock b/exercises/all-your-base/Cargo.lock
 new file mode 100644
-index 0000000..245d36d
+index 0000000..92cc4a2
 --- /dev/null
 +++ b/exercises/all-your-base/Cargo.lock
 @@ -0,0 +1,4 @@
 +[root]
-+name = "allyourbases"
++name = "allyourbase"
 +version = "0.0.0"
 +
 diff --git a/exercises/all-your-base/Cargo.toml b/exercises/all-your-base/Cargo.toml
 new file mode 100644
-index 0000000..b67d361
+index 0000000..158e712
 --- /dev/null
 +++ b/exercises/all-your-base/Cargo.toml
 @@ -0,0 +1,3 @@
 +[package]
-+name = "allyourbases"
++name = "allyourbase"
 +version = "0.0.0"
 diff --git a/exercises/all-your-base/example.rs b/exercises/all-your-base/example.rs
 new file mode 100644
@@ -159,11 +159,11 @@
 +}
 diff --git a/exercises/all-your-base/tests/all-your-base.rs b/exercises/all-your-base/tests/all-your-base.rs
 new file mode 100644
-index 0000000..fdf2ef5
+index 0000000..9f49cb5
 --- /dev/null
 +++ b/exercises/all-your-base/tests/all-your-base.rs
 @@ -0,0 +1,177 @@
-+extern crate allyourbases as ayb;
++extern crate allyourbase as ayb;
 +
 +#[test]
 +fn single_bit_one_to_decimal() {
@@ -340,3 +340,16 @@
 +    let output_base = 0;
 +    assert!(ayb::convert(input_digits, input_base, output_base).is_err());
 +}
+diff --git a/problems.md b/problems.md
+index 2e39384..e09365a 100644
+--- a/problems.md
++++ b/problems.md
+@@ -39,7 +39,7 @@ etl |  btree
+ sieve |  vector, map, while let (optional)
+ rna-transcription |  match, struct, str vs string
+ roman-numerals |  mutable, results, loops, struct, traits
+-hexadecimal |  Option, zip/fold/chars, map
++all-your-base |  Result, enumerate, fold, map
+ grade-school |  struct, entry api, Vec, Option
+ tournament |  enum, sorting, hashmap, structs
+ robot-simulator | Immutability, enum

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.

3 participants