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

transpose exercise #315

Open
wants to merge 84 commits into
base: main
Choose a base branch
from
Open

transpose exercise #315

wants to merge 84 commits into from

Conversation

Ephraim-nonso
Copy link
Contributor

@Ephraim-nonso Ephraim-nonso commented Dec 9, 2024

Closes #241

Copy link

github-actions bot commented Dec 9, 2024

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",
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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!

Copy link
Contributor

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 😅

Copy link
Contributor

@0xNeshi 0xNeshi left a 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",
Copy link
Contributor

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![];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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"];
Copy link
Contributor

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 ",
Copy link
Contributor

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 "];
Copy link
Contributor

@0xNeshi 0xNeshi Dec 11, 2024

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"difficulty": 1
"difficulty": 4

It's not a trivial exercise, let's increase the difficulty

@Ephraim-nonso
Copy link
Contributor Author

Good first iteration, let's polish it a bit!

Thanks for the review. I'd address them.

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.

Add Practice Exercise: transpose
2 participants