Skip to content

Conversation

@Hombre-x
Copy link
Contributor

@Hombre-x Hombre-x commented Aug 18, 2024

This PR mostly addresses two important things:

The first is a project that uses this library to create a small contact manager using the command line. There is both the running code in the examples and a tutorial under docs/tutorial/contact_manager.md explaining the hows and whys.

You can use it by doing cm help to display how to use the command.

Regarding the project, this is what is contained on each folder:

  • contacts/domain: The types of the application, such as Contact, CliCommand and Flag, that address the in memory representation of a contact in Scala, the command to execute in the CLI and the different update flags for the update command, respectively.

  • contacts/core: Here is the ContactManager algebra. This uses shellfish to add, remove and update the contacts on a particular file. It also has methods to search uses by their name, email and phone number.
    Regarding the implementation, it stores the contacts on a custom encoding that's basically every contact field divided by a |, and stores them line by line.

  • contacts/cli: This class has the functionality to interact with the user in the Cli, it prints to the stdout and takes user input to execute the commands; the Prompt object has functionality to parse the user input in form of flags and subcommands to convert them to instructions to the Cli.

  • contacts/app: This is basically the entry point of the application.

Any changes or doubts about the code are very welcome!

Includes the Contact type (in-memory representation of a contact), the Flags and some Arguments for the CLI
It implements the logic for the file handling and search using Shellfish
Encapsulates the logic for interacting with the cli app
Responsible for converting the user prompt in for of args: List[String] into Flags and Arguments
Now all use flatMap insead of mapN
As I forgot that Scala 2 doesn't have top level definitions
It now discards any empty characters after the last newline, just like the `wc` command in Unix

Also, added tests to assert that behavior
I forgot that `tempFile` does not exist anymore
Because readLines was modified, It now doesn't account the newline added by the writeLines method.
Comment on lines 83 to 84
} yield expect(sizeBefore + 2 == sizeAfter)
// That is, one new line of the appendLine and, one newline character of the writeLines method
Copy link
Contributor

Choose a reason for hiding this comment

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

If you've added the dropLastIf(_.isEmpty) how come you need this modification here?

Copy link
Contributor Author

@Hombre-x Hombre-x Aug 22, 2024

Choose a reason for hiding this comment

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

Before the changes:

val aList = List("a", "b")

for 
  _          <- path.writeLines(contentsList)
  sizeBefore <- path.readLines.map(_.size) // sizeBefore is 3, as writeLines adds a newline at the end
  _          <- path.appendLine("Im a last line!") // Adds another line
  sizeAfter  <- path.readLines.map(_.size) // Now sizeAfter is 4
yield expect(sizeBefore + 1 == sizeAfter) // so this condition is true

Now:

val aList = List("a", "b")

for 
  _          <- path.writeLines(contentsList)
  sizeBefore <- path.readLines.map(_.size) // sizeBefore is 2, as readLines now ignores all empty characters before the last newline
  _          <- path.appendLine("Im a last line!") // Adds another line
  sizeAfter  <- path.readLines.map(_.size) // Now sizeAfter is 4, as we didnt get rid of the newline added by writeLines
yield expect(sizeBefore + 1 == sizeAfter) // so this condition is not true anymore, as sizeBefore is 2 and sizeAfter is 4

Comment on lines 50 to 56
private def parseContact(contact: String): Either[Exception, Contact] =
contact.split('|') match {
case Array(id, firstName, lastName, phoneNumber, email) =>
Contact(id, firstName, lastName, phoneNumber, email).asRight

case _ => new Exception(s"Invalid contact format: $contact").asLeft
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Albeit IO is not strictly required, using it the following signatures will become extremely easy.

Suggested change
private def parseContact(contact: String): Either[Exception, Contact] =
contact.split('|') match {
case Array(id, firstName, lastName, phoneNumber, email) =>
Contact(id, firstName, lastName, phoneNumber, email).asRight
case _ => new Exception(s"Invalid contact format: $contact").asLeft
}
private def parseContact(contact: String): IO[Contact] =
contact.split('|') match {
case Array(id, firstName, lastName, phoneNumber, email) =>
Contact(id, firstName, lastName, phoneNumber, email).pure[IO]
case _ => new Exception(s"Invalid contact format: $contact").raiseError
}

Copy link
Contributor Author

@Hombre-x Hombre-x Sep 8, 2024

Choose a reason for hiding this comment

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

I have a question, I understand the added value of switching the signature from a lines.traverse(IO.fromEither(parseContact(_))) to a lines.traverse(parseContact) by returning an IO, but isn't it bad practice to use IO on functions that don't do I/O at all? I mean, it's like using Async on an operation that only needs ApplicativeError to be referentially transparent 🙃

Aside from that, changes committed! Thanks

Copy link
Contributor

@TonioGela TonioGela left a comment

Choose a reason for hiding this comment

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

As you might have noticed I added a lot of comments. IMHO you duplicated a lot of code and you've used a too low-level approach (i.e. when updating a contact it's easier to parse them all from disk, alter the contact you wish to alter and then re-save everything on disk instead of using the vector's index to do an update at that position).
I've added a few suggestions, reducing the code duplication, dissecting the logic in more bite-sized reusable functions, and making them work on a List[Contacts] instead that on a List[String].

P.S. I haven't reviewed the tutorial yet, mostly because I assume it relies on the actual code structure.

Hombre-x and others added 5 commits September 8, 2024 17:56
@Hombre-x Hombre-x requested a review from TonioGela September 8, 2024 23:08
@Hombre-x
Copy link
Contributor Author

Hombre-x commented Sep 8, 2024

Suggestions committed!

Copy link
Contributor

@TonioGela TonioGela left a comment

Choose a reason for hiding this comment

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

Great job, I've added a few suggestions to the tutorial text, but overall it looks good.

Can I ask you to apply the corresponding modifications to the code too?

Sorry for the almost 6 months delay (expecting a daughter and having a full time job doesn't help with schedule)

@Hombre-x
Copy link
Contributor Author

Hombre-x commented Dec 28, 2024

Changes done! Shall we merge this PR or close it and add the changes to Catscript @TonioGela ?

@TonioGela TonioGela merged commit d32f7f8 into davenverse:main Jan 11, 2025
10 checks passed
@TonioGela TonioGela deleted the site-project branch January 11, 2025 15:39
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