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

Add Gleam implementation #548

Merged
merged 3 commits into from
Jun 9, 2024
Merged

Add Gleam implementation #548

merged 3 commits into from
Jun 9, 2024

Conversation

chiroptical
Copy link
Contributor

@chiroptical chiroptical commented Jun 5, 2024

READ ME BEFORE SUBMITTING A PR

Please do not submit a PR with your solution to the Gilded Rose Kata. This repo is intended to be used as a starting point for the kata.

  • I acknowledge that this PR is not a solution to the Gilded Rose Kata, but an improvement to the template.

Please provide your PR description below this line

Hello, I would like to add a starting point for the gleam programming language https://gleam.run/

I was able to confirm that the implementation passes the text tests

➤ gleam run > ~/output.txt
➤ diff ../texttests/ThirtyDays/stdout.gr ~/output.txt
0a1,2
>    Compiled in 0.02s
>     Running gilded_rose.main
373d374
< 

I didn't add this to the text test configuration because gleam doesn't produce an executable. I am not familiar enough with the tool to know if it would support something like gleam run. Also, gleam run does produce a bit of compiler information into stdout so it might cause test text to fail.

gleam/manifest.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@codecop codecop left a comment

Choose a reason for hiding this comment

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

Looks very good otherwise. Thank you.

gleam/src/gilded_rose.gleam Outdated Show resolved Hide resolved
@chiroptical chiroptical requested a review from codecop June 6, 2024 17:39
@codecop
Copy link
Collaborator

codecop commented Jun 6, 2024

Sorry for requesting another change I did not communicate clearly. It is very important to have as much the same file structure as other languages to avoid confusion of participants which could work in different languages. I have indicated what is needed

@chiroptical
Copy link
Contributor Author

Sorry for requesting another change I did not communicate clearly. It is very important to have as much the same file structure as other languages to avoid confusion of participants which could work in different languages. I have indicated what is needed

Sorry, I looked at a few other functional programming languages and I don't really see a similar structure. Could you clarify what you want where? Or provide a language that exhibits this structure and I'll copy it.

main code to file named "Program" or "TextTestFixture"

Does this mean mv src/gilded_rose_item.gleam src/program.gleam? The literal main function needs to exist in src/gilded_rose.gleam but I am happy to move code wherever you prefer.

@chiroptical
Copy link
Contributor Author

It is also possible you mean that update_quality must exist in src/gilded_rose.gleam but the cli/simulation code can be elsewhere?

@chiroptical
Copy link
Contributor Author

9f9971e

I think you mean this.

@codecop
Copy link
Collaborator

codecop commented Jun 9, 2024

Other languages usually have 3 files

  • Gilded_rose with types. Item. Update quality
  • TextTestFixture or Program or main with all simulation code. Including dynamic days
  • Gilded_rose-test or similar name with initial test depending on language conventions. The test you have is fine
    Is this possible to create with gleam infrastructure? You mention that gilded_rose must contain the main method? By language design? Is it because the module is named like that? Let's discuss before you modify code to avoid misunderstanding

@chiroptical
Copy link
Contributor Author

You mention that gilded_rose must contain the main method?

https://github.com/emilybache/GildedRose-Refactoring-Kata/pull/548/files#diff-9cddb0c6718f48079ff3119fb817294d5d7a17c8e08984f35ecb2c0307067434R1 <- this line dictates where the main function has to be. I could, for example, modify this to program and then program.gleam would contain the main function. That would allow me to move the Item back into gilded_rose (it had to be separate due to circular dependencies). I could easily put the test fixture, simulation, and cli code into program.gleam it just didn't feel like good naming. However, I understand you are looking for more uniformity across the project.

My proposal,

  1. Change gleam.toml name to program
  2. Move Item and GildedRose back into gilded_rose.gleam
  3. Move the simulation, cli app, and main function into program.gleam
  4. Leave the test code alone.

Going to quickly confirm this is all possible. I'll add another comment when I confirm.

Thanks so much for working with me here.

@chiroptical
Copy link
Contributor Author

All of that proposal works, only one change. In the test code, I would need to move the "test main" function into test/program_test.gleam. The test/gilded_rose_item.gleam can stay. The error from gleam test looks like

> gleam test
   Compiled in 0.01s
    Running program_test.main
F
Failures:

  1) gilded_rose_test.update_quality_test: module 'gilded_rose_test'
     Values were not equal
     expected: "fixme"
          got: "foo"
     output:

Finished in 0.005 seconds
1 tests, 1 failures

so it should be clear where the test resides.

@codecop
Copy link
Collaborator

codecop commented Jun 9, 2024

Yes exactly. Just confirm, the file name will be gilded_rose.gleam and not gilded_rose_item.gleam. Yes please make the changes.

@chiroptical
Copy link
Contributor Author

Yes exactly. Just confirm, the file name will be gilded_rose.gleam and not gilded_rose_item.gleam. Yes please make the changes.

Yup! It will contain the Item and GildedRose types and the update_quality function.

@chiroptical
Copy link
Contributor Author

chiroptical commented Jun 9, 2024

In theory, all set. Let me know if you want me to squash and force push or anything. Thanks for working with me on this.

Any interest in me writing a CONTRIBUTING.md quick (in another PR)? Might help clear up any confusion when adding languages?

@codecop codecop merged commit 717e2a8 into emilybache:main Jun 9, 2024
@codecop
Copy link
Collaborator

codecop commented Jun 9, 2024

Thank you. Yes if you can provide a CONTRIBUTING.md, please have a separate PR for that.

@chiroptical chiroptical deleted the chiroptical/add-gleam-implementation branch June 9, 2024 19:38
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.

2 participants