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

✨ (concepts) Add concept exercise annalyns-infiltration #1668

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

devkabiir
Copy link

This concept is inspired from the same in csharp track

@devkabiir devkabiir force-pushed the concept-booleans branch 3 times, most recently from 0577327 to 0312378 Compare April 7, 2023 15:04
{
"slug": "annalyns-infiltration",
"name": "Annalyn's Infiltration",
"uuid": "897fa2ea-c29c-4d4e-9469-ff661a9b838a",
Copy link
Author

Choose a reason for hiding this comment

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

I mostly copied csharp track and generated a uuid v4. Is there anything else that needs to be done to enable this concept?

Copy link
Member

@dem4ron dem4ron Apr 8, 2023

Choose a reason for hiding this comment

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

Unfortunately concept exercises just got disabled for Rust. The syllabus will need a complete rework, so it might take some time to enable this concept exercise. (CC @ErikSchierboom)

Copy link
Author

Choose a reason for hiding this comment

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

No problem, this is exactly why I'm implementing simpler concept exercises. I only wanted to know if there are any other config files to touch to prepare it for enabling.

Copy link
Member

Choose a reason for hiding this comment

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

We will show in-progress exercise for track maintainers though.

@devkabiir devkabiir force-pushed the concept-booleans branch 3 times, most recently from 6ef3f8d to bcbc65c Compare April 7, 2023 15:23
@devkabiir devkabiir marked this pull request as ready for review April 7, 2023 15:25
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Really good! Just a couple of comments

config.json Outdated Show resolved Hide resolved
{
"slug": "annalyns-infiltration",
"name": "Annalyn's Infiltration",
"uuid": "897fa2ea-c29c-4d4e-9469-ff661a9b838a",
Copy link
Member

Choose a reason for hiding this comment

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

We will show in-progress exercise for track maintainers though.

@devkabiir devkabiir force-pushed the concept-booleans branch 4 times, most recently from 9550d96 to f1d0cb6 Compare April 14, 2023 20:37
exercises/concept/annalyns-infiltration/.meta/exemplar.rs Outdated Show resolved Hide resolved
exercises/concept/annalyns-infiltration/.meta/exemplar.rs Outdated Show resolved Hide resolved
exercises/concept/annalyns-infiltration/.meta/exemplar.rs Outdated Show resolved Hide resolved
exercises/concept/annalyns-infiltration/.meta/exemplar.rs Outdated Show resolved Hide resolved
Comment on lines +14 to +17
_knight_is_awake: bool,
_archer_is_awake: bool,
_prisoner_is_awake: bool,
_pet_dog_is_present: bool,

Choose a reason for hiding this comment

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

Is silencing the unused variable warning really desirable?

Copy link
Contributor

Choose a reason for hiding this comment

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

It has been done this way for many exercises on the Rust track. I'm not a huge fan of it personally either. One way to suppress the warning to use it in the unimplemented macro:

unimplemented!("use {knight_is_awake} to solve the exercise")

Choose a reason for hiding this comment

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

That might be preferable. I expect many/most students will just use the names provided.

Also, showing that warning here might be good pedagogy.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed.

Choose a reason for hiding this comment

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

@iHiD Is there an Exercism-wide policy here?

If (concept) exercise stubs are

  • supposed to be free of warnings, then I propose using @remlse's workaround (track-wide).
  • allowed to induce warnings, then I propose to just remove the initial underscores (track-wide) – possibly also for practice exercises.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have an Exercism-wide policy. I don't mind warnings personally, and in this case they can actually help the student as they make it clear that the student has to do something. I wonder if there is some CI running that will balk at warnings.

Choose a reason for hiding this comment

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

Let's find out.

Suggested change
_knight_is_awake: bool,
_archer_is_awake: bool,
_prisoner_is_awake: bool,
_pet_dog_is_present: bool,
knight_is_awake: bool,
archer_is_awake: bool,
prisoner_is_awake: bool,
pet_dog_is_present: bool,

@devkabiir devkabiir force-pushed the concept-booleans branch 3 times, most recently from 79ba19e to be86eca Compare May 1, 2023 19:51
This concept is inspired from the same in csharp track
Comment on lines +14 to +17
_knight_is_awake: bool,
_archer_is_awake: bool,
_prisoner_is_awake: bool,
_pet_dog_is_present: bool,

Choose a reason for hiding this comment

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

Let's find out.

Suggested change
_knight_is_awake: bool,
_archer_is_awake: bool,
_prisoner_is_awake: bool,
_pet_dog_is_present: bool,
knight_is_awake: bool,
archer_is_awake: bool,
prisoner_is_awake: bool,
pet_dog_is_present: bool,

unimplemented!("Implement can_fast_attack");
}

pub fn can_spy(_knight_is_awake: bool, _archer_is_awake: bool, _prisoner_is_awake: bool) -> bool {

Choose a reason for hiding this comment

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

Suggested change
pub fn can_spy(_knight_is_awake: bool, _archer_is_awake: bool, _prisoner_is_awake: bool) -> bool {
pub fn can_spy(knight_is_awake: bool, archer_is_awake: bool, prisoner_is_awake: bool) -> bool {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is off-topic for this PR. Both approaches are currently being used on the track. I agree we should make this consistent and introduce some CI test to make sure it stays consistent, but this PR is not the place to do it. Let's keep the underscores for now.

unimplemented!("Implement can_spy");
}

pub fn can_signal_prisoner(_archer_is_awake: bool, _prisoner_is_awake: bool) -> bool {

Choose a reason for hiding this comment

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

Suggested change
pub fn can_signal_prisoner(_archer_is_awake: bool, _prisoner_is_awake: bool) -> bool {
pub fn can_signal_prisoner(archer_is_awake: bool, prisoner_is_awake: bool) -> bool {

@@ -0,0 +1,20 @@
pub fn can_fast_attack(_knight_is_awake: bool) -> bool {

Choose a reason for hiding this comment

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

Suggested change
pub fn can_fast_attack(_knight_is_awake: bool) -> bool {
pub fn can_fast_attack(knight_is_awake: bool) -> bool {

@@ -19,18 +75,43 @@ values into a boolean value.

Unlike some other languages, `0` is not `false` and non-zero is not `true`.
Copy link
Contributor

@senekor senekor May 6, 2023

Choose a reason for hiding this comment

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

A student may wonder, what happens if a number value is used as a boolean? Is that a runtime panic?

Might be good to note here that it is a compile time error and the programmer is helpfully prevented from making this mistake.

@@ -19,18 +75,43 @@ values into a boolean value.

Unlike some other languages, `0` is not `false` and non-zero is not `true`.

Rust supports three [logical binary operators][logical binary operators]: `&` (Logical And), `|` (Logical Or), `^` (Logical Xor).
Rust supports three [logical binary operators][logical binary operators]: `&`
Copy link
Contributor

Choose a reason for hiding this comment

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

"binary operator" usually means "an operator that takes two operators". (as opposed to unary and ternary operators).

I suggest "bitwise operator" as a less ambiguous term here.

Rust supports two [boolean operators][lazy boolean operators]: `||` (Or), `&&` (And). They differ from `|` and `&` in that the right-hand operand
is only evaluated when the left-hand operand does not already determine the result of the expression. That is, `||` only evaluates
its right-hand operand when the left-hand operand evaluates to `false`, and `&&` only when it evaluates to `true`.
The operators `||` (OR), `&&` (AND) differ from `|` and `&`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should omit any mention of the bitwise operators in the concept about booleans. I've only ever seen bitwise operators used with unsigned integer types, where they operate on bit patterns.

For booleans, as is correctly described here, the difference between | and || etc. is very small - it's ony about the lazy evaluation. And a program which relies on lazy / eager evaluation of it's boolean expressions because the operands are side-effectful... is a terrible program in my opinion.

In other languages, it may be common to use the lazy boolean operators for control flow, but not in Rust.

With all that in mind, talking about the bitwise operators for booleans at all seems confusing at best, and suggestive of bad practices at worst.

Opinions? @MatthijsBlom

Choose a reason for hiding this comment

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

I fully agree that some Operations on Bits concept node would be a better place for introducing the bitwise operators.


And a program which relies on lazy / eager evaluation of it's boolean expressions because the operands are side-effectful... is a terrible program in my opinion.

Not exactly a counterexample – because it does not involve side effects – but: e.g. if some_list and predicate(some_list[0]) is a fairly common construct in Python. The some_list checks that it is not empty; only if it nonempty is some_list[0] looked up, avoiding a runtime error.

More generally, successful evaluation of subsequent propositions might depend on prior propositions holding.

let f: bool = false; // with explicit type annotation
```

The main way to use Boolean values is through conditionals, such as an if
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the exact purpose of about.md and introduction.md on a concept? There is a lot of duplicated text between the two files, is that normal? I guess would be that some exaplantion should go in one of the two files - not in both.

@ErikSchierboom

Choose a reason for hiding this comment

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

These are documented here: https://exercism.org/docs/building/tracks/concepts

introduction.md is shown to students who have not yet solved the concept exercise(s?), and about.md is shown to those who have.

It is entirely normal for the two to share a lot of text. It also happens that they are identical.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that's entirely correct.

let f: bool = false; // with explicit type annotation
```

The main way to use Boolean values is through conditionals, such as an if
Copy link
Contributor

Choose a reason for hiding this comment

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

There is even more duplication here. The text explaining booleans seems to be copied three times? in the concept's about, the concept's introduction and here in the exercise's introduction.

Choose a reason for hiding this comment

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


- Know of the existence of the `bool` type and its two values.
- Know about boolean operators and how to build logical expressions with them.
- Know of the boolean operator precedence rules.
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, the concept introduction does not explain the precedence rules. Let's add that.

@senekor
Copy link
Contributor

senekor commented Dec 14, 2023

The author of this PR has stopped responding in the discussions planning the work on the syllabus. But there's been a lot of work put into reviews already, so I'm keeping it open in case it can be salvaged in a future attempt to create a good syllabus.

@ErikSchierboom
Copy link
Member

For the record, I'd like to state that pacman-rules is usually the better choice to teach booleans and boolean expressions.

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.

5 participants