-
Notifications
You must be signed in to change notification settings - Fork 524
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
Example of Luhn parts 2 & 3, From Trait and Custom Trait #248
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, because people will learn about implementing traits on primitives and learn it is not a good idea but can be useful sometimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really wish that it would just work if we add arbitrary slugs to config.json
.
I applied the following diff to trackler
diff --git c/fixtures/tracks/fake/config.json i/fixtures/tracks/fake/config.json
index dea26e6..b942964 100644
--- c/fixtures/tracks/fake/config.json
+++ i/fixtures/tracks/fake/config.json
@@ -21,6 +21,11 @@
]
},
{
+ "slug": "my-new-awesome-problem" ,
+ "difficulty": 1,
+ "topics": []
+ },
+ {
"difficulty": 3,
"slug": "one",
"topics": [
diff --git c/fixtures/tracks/fake/my-new-awesome-problem/HINTS.md i/fixtures/tracks/fake/my-new-awesome-problem/HINTS.md
new file mode 100644
index 0000000..b53cb5e
--- /dev/null
+++ i/fixtures/tracks/fake/my-new-awesome-problem/HINTS.md
@@ -0,0 +1 @@
+My new awesome problem.
diff --git c/fixtures/tracks/fake/my-new-awesome-problem/test.a i/fixtures/tracks/fake/my-new-awesome-problem/test.a
new file mode 100644
index 0000000..ce01362
--- /dev/null
+++ i/fixtures/tracks/fake/my-new-awesome-problem/test.a
@@ -0,0 +1 @@
+hello
diff --git c/test/trackler/implementation_test.rb i/test/trackler/implementation_test.rb
index 8b39d2b..0afe821 100644
--- c/test/trackler/implementation_test.rb
+++ i/test/trackler/implementation_test.rb
@@ -73,6 +73,18 @@ class ImplementationTest < Minitest::Test
refute implementation.exists?, "Not expecting apple to be implemented for track TRACK_ID"
end
+ def test_unknown_problem
+ slug = 'my-new-awesome-problem'
+ problem = Trackler::Problem.new(slug, PATH)
+ implementation = Trackler::Implementation.new(TRACK_ID, URL, problem, PATH)
+ begin
+ puts implementation.files
+ rescue => e
+ puts "Aw that's a shame, didn't find the files of #{slug} because #{e}"
+ end
+ assert implementation.exists?, "Expecting #{slug} to be implemented"
+ end
+
def test_override_implementation_files
implementation = Trackler::Implementation.new(TRACK_ID, URL, PROBLEM, PATH)
files = { "filename" => "contents" }
then I ran the tests and got the following result:
$ ruby trackler/implementation_test.rb (01-17 00:14)
Run options: --seed 23017
# Running:
.........Aw that's a shame, didn't find the files of my-new-awesome-problem because undefined method `[]' for nil:NilClass
F...
Fabulous run in 0.009562s, 1359.6007 runs/s, 2091.6933 assertions/s.
1) Failure:
ImplementationTest#test_unknown_problem [trackler/implementation_test.rb:85]:
Expecting my-new-awesome-problem to be implemented
13 runs, 20 assertions, 1 failures, 0 errors, 0 skips
Let me know if you think I did the test wrong in some way. But I predict this means the new problems will not actually exist.
exercises/luhn-from/HINTS.md
Outdated
|
||
`exercism fetch rust luhn` | ||
|
||
In the original Luhn exercise you only validated strings, but the Luhn algorithm can be applied integers as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"applied to"
exercises/luhn-trait/HINTS.md
Outdated
- `exercism fetch rust luhn` | ||
- `exercism fetch rust luhn-from` | ||
|
||
In the original Luhn exercise you only validated strings, but the Luhn algorithm can be applied integers as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"applied to"
exercises/luhn-trait/HINTS.md
Outdated
|
||
Instead of creating a Struct just to perform the validation, what if you you validated the primitives (i.e, String, u8, etc.) themselves? | ||
|
||
In this exercise you'll create and implement a custom [trait](https://doc.rust-lang.org/book/traits.html) that performs the validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
End sentence with a period, I think.
I am unfamiliar with Trackler. Here's what I see after setting it up and pointing its version of the Rust track to this branch. rust_track = Trackler.tracks["rust"]
implementations = rust_track.implementations
luhn_from = implementations.detect {|impl| impl.problem.slug == 'luhn-from'}
# => #<Trackler::Implementation:0x007fa6101799f0 @track_id="rust",...>
luhn_from.files
# => NoMethodError: undefined method `[]' for nil:NilClass The error is thrown here, because the metadata method returns nil. I think that's a fixable problem in Trackler. What it's trying to do here is pull together the language-independent files for an exercise. But if those files don't exist there should be fall-back behavior. I think. I mean, I've only looked at that code for 5 minutes so I'm sure there's complexity I'm missing. I wanna dig into it more. Will have to wait until Wednesday at the earliest, though. |
Not sure when it'll be deployed, but track-specific exercises should be possible soon. |
config.json
Outdated
@@ -132,6 +132,26 @@ | |||
] | |||
}, | |||
{ | |||
"slug": "luhn-from", | |||
"difficulty": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that I gave Luhn a 4, perhaps this should be as well
config.json
Outdated
}, | ||
{ | ||
"slug": "luhn-trait", | ||
"difficulty": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that I gave Luhn a 4, perhaps this should be as well
same
exercises/luhn-from/example.rs
Outdated
} | ||
|
||
impl From<&'static str> for Luhn { | ||
fn from(s: &'static str) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it's only the example, but I'm not clear on why this would have to be &'static str
specifically. What would happen if this were just &str
? It seems unnecessarily restrictive to me to only accept &'static str
.
exercises/luhn-trait/HINTS.md
Outdated
|
||
In the original Luhn exercise you only validated strings, but the Luhn algorithm can be applied to integers as well. | ||
|
||
In Luhn From you implemented a From trait, which also required you to create a Luhn struct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here, it says "Luhn From", whereas above it says "Luhn From Trait". In the header of the HINTS.md
of the luhn-from
, it says "Luhn with From Trait". Intentional? Seems OK I guess since they should all be distinguishable.
exercises/luhn-trait/example.rs
Outdated
} | ||
} | ||
|
||
impl Luhn for &'static str { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same - I'd ask if there is any requirement that these be &'static str
rather than just &str
.
bb5e7d1
to
ed4d770
Compare
Rebased on master and made @petertseng's requested changes. I'm not sure if the updated Trackler is deployed yet, so I don't know what will happen if this gets deployed. |
26ded73
to
d2a257f
Compare
Nearly done. After rebasing down to a single commit I realized that I had the Luhn followups in the wrong part of the track. "Luhn: Using the From Trait" now moves to just after Bracket Push (which is our example that uses "Luhn: Using a Custom Trait" moves to after Space Age. Space Age solutions use custom traits and |
d2a257f
to
5c61101
Compare
Checked with Katrina. Trackler is updated, so merging this is fine. |
This implements the idea I discussed here exercism#247 (comment) After students do the original Luhn exercise, they can now do "Luhn: Using the From Trait" to allow validation of multiple types. And after that, they can do "Luhn: Using a Custom Trait" to develop their own trait. "Luhn: Using the From Trait" is placed just after Bracket Push (which also uses `From`). In the Luhn implementation students will implement `From` for multiple types, so it makes sense to put it after Bracket Push where students only implement it for one type. "Luhn: Using a Custom Trait" moves to after Space Age. Space Age solutions use custom traits and `From`, so pairing these exercises made sense.
5c61101
to
68caa48
Compare
@IanWhitney trackler assumes that there's a |
They were created for #248, but since they're not in the ignore_pattern they are being served. See for proof: http://x.exercism.io/v2/exercises/rust/luhn-from http://x.exercism.io/v2/exercises/rust/luhn-trait And note how description.md and metadata.yml appear inside the response.
Closes exercism#248 Approached a little different this time now that the core set of concepts start crystallizing. I've taken two reasonably different solutions and extracted applied language components. There's some overlap between some concepts (like `Ordering` and `conditionals`), but I think it's reasonable to touch such concepts in different concept-exercises.
Closes exercism#248 Approached a little different this time now that the core set of concepts start crystallizing. I've taken two reasonably different solutions and extracted applied language components. There's some overlap between some concepts (like `Ordering` and `conditionals`), but I think it's reasonable to touch such concepts in different concept-exercises.
Closes exercism#248 Approached a little different this time now that the core set of concepts start crystallizing. I've taken two reasonably different solutions and extracted applied language components. There's some overlap between some concepts (like `Ordering` and `conditionals`), but I think it's reasonable to touch such concepts in different concept-exercises.
Closes exercism#248 Approached a little different this time now that the core set of concepts start crystallizing. I've taken two reasonably different solutions and extracted applied language components. There's some overlap between some concepts (like `Ordering` and `conditionals`), but I think it's reasonable to touch such concepts in different concept-exercises.
This is an implementation of the idea I discussed here
#247 (comment)
After students do the original Luhn exercise, they can learn how to use
the From trait to allow validation of multiple types. Then they can implement it again with a custom type.