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

docs: use parentheses in sublist instructions example #507

Closed
wants to merge 2 commits into from

Conversation

nlevchuk
Copy link

Hello,

First of all, thanks all contributors for interesting exercises.

Determining set of items as clojure list (not as vector) seems right because this task operates in terms of lists.

This task operates in terms of lists and thus, determining set of items as clojure list (not as vector) seems right
@bobbicodes
Copy link
Member

Hi! I'm just catching up on some of these tickets, for some reason I missed this one until now.

While it is slightly confusing, I'm thinking that "list" here really means "vector", as evidenced by the test suite. Lists need to be quoted or else they are considered function calls, and for this reason vectors are nearly always used in Clojure to represent data.

I think the best thing to do would be to add a note to the instructions explaining this.

@tasxatzial
Copy link
Member

tasxatzial commented Jul 6, 2023

Since something like A = [1 2 3] in the problem description isn't really meant to be interpreted as Clojure code, I believe that a "list" could be equally represented as [1 2 3] or (1 2 3). However, the problem description reads Given two lists determine whether.... This might confuse people and lead them to an implementation that works with lists, but fails with vectors.

Maybe add a simple note in the rules section to let people know that the function should accept either a Clojure list or a vector? Everything else can remain unchanged.

Edit: I'm proposing something like this because having a function that works with both vectors and lists makes people think how they should write code that works with different types of collections instead of just one.

Edit2: Currently the tests always assume vectors and therefore people won't have a way to test whether their code works with lists and/or vectors. Also, adding tests that work with lists isn't a good idea since this will break some of the existing solutions. I guess my proposal can be disregarded.

@bobbicodes
Copy link
Member

bobbicodes commented Jul 9, 2023

It's kind of like how Clojure vectors are similar to arrays in other languages, so when exercises are ported that deal with arrays (e.g. Bird watcher), it's assumed to be synonymous. So in this case, I'm in favor of adding a note to that effect, that while the exercise is called sublist, in Clojure lists are very seldom used for data, they are mostly for code (functions), and for the purposes of the exercise, "list" should be interpreted as "vector". The alternative would be to rename the exercise "Subvec", but I don't want to do that.

@tasxatzial
Copy link
Member

tasxatzial commented Jul 9, 2023

Immediately after my proposal, i realized that the bracket notation essentially comes from the problem specs and it's used as a standard notation for lists across tracks. Since this issue probably appears in other files too, i would have chosen not to change them at all. I also agree that this note is more appropriate.

That said, even though vectors are typically used for data storage, a proper sublist implementation should ideally work with both vectors and lists because we don't know where the arguments come from. They could be vectors, but nothing stops them from being lists. That's how most Clojure functions work, for example we can write (= [1 2 3] '(1 2 3)) and this works just fine. I recall also coming across at least one exercise that uses vectors in tests, but also tests with lists.

This is something to keep in mind for future exercises. As i previously said, sometimes it's good to force people to come up with implementations that raise the level of abstraction.

@bobbicodes
Copy link
Member

That's a good point that the tests don't care about collection type, only values.

@tasxatzial
Copy link
Member

tasxatzial commented Jul 10, 2023

I'm a little worried that the "But in Clojure lists are rarely used to represent data" might be confusing. Lists are often used and there isn't sufficient explanation on when to use vectors vs lists. This might make some people start using vectors everywhere. Or, maybe they'll start asking questions how to select between vectors and lists.

I believe this kind of statement should probably be mentioned and explained in a concept, not here. @bobbicodes Your thoughts?

@bobbicodes
Copy link
Member

Yeah, I think I agree. I suppose we could just leave it the way it is.

@tasxatzial
Copy link
Member

You mean not commit anything at all? Here's another idea. What if let people decide how they want to approach this:

Note: This exercise is called "sublist" to remain consistent across tracks. You may assume that the input is a Clojure vector, but feel free to provide an implementation that works with lists too.

@bobbicodes
Copy link
Member

That seems reasonable to me.

@nlevchuk
Copy link
Author

nlevchuk commented Jul 10, 2023

Hi! Thank you for jumping into the PR)

I suppose I read the Examples part very literally, which may be wrong. It's more pseudocode rather than real clojure code, and looking through some other instructions files in the repository proves it. I think we can leave the Examples structure in the original state but add a minor note in its title: Examples (in pseudocode) or something like this.

Since this exercise is labeled as Easy, I wouldn't mix up two concepts (List and Vector) together. Somebody, especially beginners, might not be familiar with the concept of Vectors before executing the Bird Watcher exercise because it is explained there for the first time.

What do you think?

@tasxatzial
Copy link
Member

tasxatzial commented Jul 10, 2023

I think we can leave the Examples structure in the original state but add a minor note in its title: Examples (in pseudocode) or something like this.

Isn't it obvious that something like A = [1, 2, 3] is not Clojure? It does not have any parentheses.

@bobbicodes
Copy link
Member

The additional practice exercises linked in the syllabus are meant to be just suggestions, i.e. optional, so I never considered that having them there would be a negative thing. But if it's a point of confusion I'd be open to pruning them.

@nlevchuk
Copy link
Author

@tasxatzial

Isn't it obvious that something like A = [1, 2, 3] is not Clojure? It does not have any parentheses.

Well, yes and no. Commas are treated as whitespaces and optional but they can still be used. That's why it's confusing me in the beginning: is it just an array (data type) or a clojure vector that uses commas as a separator. But obviously, that's not something beginners in Clojure should be aware of.

For me, provided examples are definitely useful and mean something based on the information I've learned so far. I believe there would have been no questions if those examples were presented in terms of Clojure lists. For instance, displaying a few examples of the function's signatures:

(classify '(1 2 3) '(1 2 3 4 5))
;; => sublist

(classify '(1 2 3 4 5) '(2 3 4))
;; => superlist

@bobbicodes

i.e. optional, so I never considered that having them there would be a negative thing.

It's not a negative thing at all. But maybe putting it in an appropriate form would improve the situation

@tasxatzial
Copy link
Member

The additional practice exercises linked in the syllabus are meant to be just suggestions, i.e. optional, so I never considered that having them there would be a negative thing. But if it's a point of confusion I'd be open to pruning them.

I believe that the answer to this is rather simple: Do you feel that somebody who completes the introductory list exercise should be able to complete the sublist too? The current prerequisite is just the concept exercise.

@tasxatzial
Copy link
Member

tasxatzial commented Jul 10, 2023

I believe there would have been no questions if those examples were presented in terms of Clojure lists

If you take a look at the tests you'll see that they use vectors instead of lists. People will start asking why lists are used in the examples. But tests can't be changed to lists because this would break the existing solutions. Therefore changing everything in the instructions to a Clojure list notation would also require an extra note at the end:

For this exercise you may assume that the input is a Clojure vector, but feel free to provide an implementation that works with lists too.

So I don't believe there's a way to avoid vectors here. That said, i think it makes sense to change the examples to use the Clojure list notation because it does feel a bit awkward to say list A = [1,2,3] using the Clojure vector notation even though this isn't really Clojure code.

@nlevchuk
Copy link
Author

nlevchuk commented Jul 11, 2023

But tests can't be changed to lists because this would break the existing solutions.

Why do you think so? This example (sublist/classify '() '()) from tests works fine but perhaps I overlooked something

@tasxatzial
Copy link
Member

It may work just fine with a particular solution, but since the current tests never use lists, it's expected that some people have supplied solutions that work with vectors only. Those solutions will be completely broken.

@tasxatzial tasxatzial self-assigned this Sep 9, 2024
@tasxatzial
Copy link
Member

Since any sync with the problem specs will overwrite changes made to the instructions, it's probably best to have an extra .append.md file to clarify the issue — nothing more. I'd prefer to do a separate PR for it, though.

@nlevchuk
Copy link
Author

nlevchuk commented Sep 9, 2024

@tasxatzial Hi. I'm afraid, I don't have the time to delve into this issue at the moment. If no one else has reported the same problem, it might be best to close this PR. Thank you

@tasxatzial
Copy link
Member

tasxatzial commented Sep 9, 2024

@nlevchuk No problem, i'll create a new PR. Thanks for replying.

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