-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
transpose exercise #315
base: main
Are you sure you want to change the base?
transpose exercise #315
Conversation
Hello 👋 Thanks for your PR. This repo does not currently have dedicated maintainers. Our cross-track maintainers team will attempt to review and merge your PR, but it will likely take longer for your PR to be reviewed. If you enjoy contributing to Exercism and have a track-record of doing so successfully, you might like to become an Exercism maintainer for this track. Please feel free to ask any questions, or chat to us about anything to do with this PR or the reviewing process on the Exercism forum. (cc @exercism/cross-track-maintainers) |
@@ -820,6 +820,14 @@ | |||
"prerequisites": [], | |||
"difficulty": 4 | |||
}, | |||
{ | |||
"slug": "word-count", |
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.
You already added word-count
, see https://github.com/exercism/cairo/pull/304/files
Delete these files from the PR.
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.
Apologies for this error. I had issues which prompted this error along the line. I'd delete it from the PR.
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 have deleted these files from the PR. Please, let me know if there are other changes to make.
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.
No problem at all, it happens!
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.
The files are still in the PR 😅
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.
Good first iteration, let's polish it a bit!
@@ -820,6 +820,14 @@ | |||
"prerequisites": [], | |||
"difficulty": 4 | |||
}, | |||
{ | |||
"slug": "word-count", |
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.
The files are still in the PR 😅
|
||
#[test] | ||
fn empty_string() { | ||
let mut input: Array<ByteArray> = array![]; |
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.
let mut input: Array<ByteArray> = array![]; | |
let input: Array<ByteArray> = array![]; |
no need for input
to be mutable, address this in all test cases
#[ignore] | ||
fn simple() { | ||
let mut input: Array<ByteArray> = array!["ABC", "123"]; | ||
let expected = array!["A1", "B2", "C3"]; |
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.
nit: let's format these row values into rows, and column values into columns.
You can use #[cairofmt::skip]
to tell the formatter to not touch this then.
This is similar to what we've done for minesweeper.
#[cairofmt::skip]
let input: Array<ByteArray> = array![
"ABC",
"123",
];
#[cairofmt::skip]
let expected = array![
"A1",
"B2",
"C3",
];
Note: apply this change to all tests where the formatter didn't already format the values like this
"lnnn", | ||
"ogge", | ||
"n e.", | ||
"glr ", |
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.
You should not pad to the right, i.e. add padding spaces to the rightmost columns (as per the problem description).
Copy/paste the actual expected output from the canonical data
#[ignore] | ||
fn jagged_triangle() { | ||
let mut input: Array<ByteArray> = array!["11", "2", "3333", "444", "555555", "66666"]; | ||
let expected = array!["123456", "1 3456", " 3456", " 3 56", " 56", " 5 "]; |
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.
let expected = array!["123456", "1 3456", " 3456", " 3 56", " 56", " 5 "]; | |
let expected = array!["123456", "1 3456", " 3456", " 3 56", " 56", " 5"]; |
Same as before, no padding to the right is allowed (see canonical data)
let mut i = 0; | ||
loop { | ||
let mut temp: ByteArray = ""; | ||
for line in input |
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.
It must be possible to avoid having a loop + for + break + continue
combination to perform this operation, makes it very hard to follow what is going on.
Find a way to refactor this into something more readable.
You can use Go implementation as inspiration, it is very clear and concise
"uuid": "59b5d30b-62c1-4465-a44d-485c45f4aac2", | ||
"practices": [], | ||
"prerequisites": [], | ||
"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.
"difficulty": 1 | |
"difficulty": 4 |
It's not a trivial exercise, let's increase the difficulty
Thanks for the review. I'd address them. |
Closes #241