Navigation Menu

Skip to content
This repository has been archived by the owner on Oct 7, 2022. It is now read-only.

Redis haskell support #26

Merged
merged 6 commits into from Jun 14, 2020

Conversation

nixypanda
Copy link
Contributor

@nixypanda nixypanda commented Jun 13, 2020

Few things to note:

  1. This is a stack project so someone can install all the required dependencies that they would like. With haskell people would like to use attoparsec, parsec, megaparsec etc. or some other parser for RESP parsing and I don't really want to restrict people from using any of those libs. If someone is putting in the effort to do this course, they should be sensible enough to, not cheat.
  2. The uncommenter should not have the check for each and every source file. i.e. that operates under the assumption that you will have only one source file. Where as Haskell stack projects have multiple such files. I think uncommenter should operate without throwing exceptions. Maybe it can give out a warning log on source files in which it did not find the Uncomment this string.
  3. I don't really know why there is a permission issue happening on the ci pipeline. It works completely fine on my local setup by doing make test_redis_haskell!

@nixypanda nixypanda changed the title Redis haskell support [WIP] Redis haskell support Jun 13, 2020
@nixypanda nixypanda changed the title [WIP] Redis haskell support Redis haskell support Jun 13, 2020
@rohitpaulk
Copy link
Member

We'll need to find a way to suppress these logs @sherubthakur

Screenshot 2020-06-14 at 2 07 41 PM

Although not relevant to the Redis challenge, other challenges will likely need to make assertions on program output (like the Git challenge). Maybe stack run has a --silent option or something like that? Or maybe run stack build piped to dev/null and then run that binary?

@rohitpaulk
Copy link
Member

And I'll look into the permissions error..

@rohitpaulk
Copy link
Member

Also, let's remove the test directory, keeping the code footprint minimal on purpose. We might work on first-class support for tests etc. in the future, but I think it's best if that is consistent across languages when it happens.

@rohitpaulk
Copy link
Member

rohitpaulk commented Jun 14, 2020

This is a stack project so someone can install all the required dependencies that they would like. With haskell people would like to use attoparsec, parsec, megaparsec etc. or some other parser for RESP parsing and I don't really want to restrict people from using any of those libs. If someone is putting in the effort to do this course, they should be sensible enough to, not cheat.

👍 Yh, that's fine for now. I actually don't mind adding dependencies, people can always work their way around it adding source files directly. The problem with doing so is build caching - our builds are already slow (~20s), and the feedback loop is a real productivity-killer. Allowing dependencies will aggravate this problem further.

In regards to how we'll prevent "gaming" this, sometime in the future you'll see an option to mark certain files as "system-owned", and commits will be rejected if they make changes to those file. That way, we'll be able to lock dependencies - just bundle the "standard" ones and call it a day.

As you said though, wouldn't worry about it now - just sometime down the line.

@rohitpaulk
Copy link
Member

The uncommenter should not have the check for each and every source file. i.e. that operates under the assumption that you will have only one source file. Where as Haskell stack projects have multiple such files. I think uncommenter should operate without throwing exceptions. Maybe it can give out a warning log on source files in which it did not find the Uncomment this string.

Yh, I agree - if multiple source files is a genuine use-case, the uncommenter shouldn't work that way. If we remove tests from this, is there a use-case for multiple source files? If so, I'll make changes to the uncommenter logic.

@nixypanda
Copy link
Contributor Author

Also, let's remove the test directory, keeping the code footprint minimal on purpose. We might work on first-class support for tests etc. in the future, but I think it's best if that is consistent across languages when it happens.

This is something I would love to see. i.e. all the repos have a testing setup already in place. For now I will remove this logic.

Yh, I agree - if multiple source files is a genuine use-case, the uncommenter shouldn't work that way. If we remove tests from this, is there a use-case for multiple source files? If so, I'll make changes to the uncommenter logic.

Tests is the only genuine use case that I can think of at the moment. So when we add tests folder to projects then we will need to update this. i.e. to a logic like "at-lest one source file gets uncommented".

@rohitpaulk rohitpaulk merged commit dbd6123 into codecrafters-io:master Jun 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants