Add a new scale #7

AryanGodara · 2023-03-17T14:02:50Z

Resolves #3 (May add more scales if this one works properly)
Review needed, not ready yet

abbysmal · 2023-03-17T14:19:48Z

cardio_crumble.opam

@@ -13,8 +13,6 @@ depends: [
  "ocaml"
  "dune" {>= "3.3"}
  "odoc" {with-doc}
-  "portmidi"


I don't think this should go away?

(I did not review the rest yet, will take a look maybe later today or this week end! (or @pitag-ha may take a look before!) But this is great, thank you very much!)

Thanks for the review @Engil !
It seems like I deleted this accidently xD while going through all the files, and checking dependencies etc.

@Engil , just checked, this is weird
When I run,

dune build

It removes portmidi automatically. Same with cmdliner

Ha, I am silly!
The dependencies should be added to dune-project, not the .opam file.
Dune seems to be handling dependencies by itself, I was not aware of this behaviour.

The dependencies should be added to dune-project, not the .opam file. Dune seems to be handling dependencies by itself

We've specified that we want Dune to do so by having (generate_opam_files true) in the dune-project.

@AryanGodara, do you want to fix this for us by adding the two dependencies to the dune-project file (and re-adding them to the .opam-file)? You could have a look at the dune-project file of the portmidi library to see the syntax. No pressure! I'm happy to do it myself if you prefer to focus on the scale task.

No I'd definitely like to work on this issue @pitag-ha !!
Should I open a separate issue for this? Or increase the scope of this current PR?

The change seems trivial in size (although I might get it wrong hehe), so I'll push another commit to this PR for it.

Also, can you please review the code @pitag-ha, when you get the time, since there are no CI checks, I can't tell if I'm moving in the right direction (The code does seem to run, when using --scale=minor or --scale=pentatonic, but I'm afraid my ears arent' that trained to catch if it's working properly xd)

No I'd definitely like to work on this issue @pitag-ha !!

Nice!

Should I open a separate issue for this? Or increase the scope of this current PR?

I leave that up to you. The cleaner version would be to open a separate PR, merge it, and then rebase this branch over the branch of the other PR. However, as you've said, this change is minimal, so mixing the things here is ok as well. If you think otherwise about git history pedantry, @Engil, let us know!

pitag-ha

Thanks, @AryanGodara, this is a great first contribution!

Apart from the few comments I have below (and the one about the dependencies is clearly on us), it looks all good to me to be merged.

.ocamlformat

bin/midi.ml

.gitignore

pitag-ha

Thanks for the changes, @AryanGodara!

bin/midi.mli

cardio_crumble.opam

AryanGodara · 2023-03-20T09:47:15Z

@pitag-ha , I've updated the new changes, sorry for the delay!

pitag-ha · 2023-03-21T12:20:35Z

I've updated the new changes,

Thanks! To be a bit clearer on the dependencies: Given that you're fixing the dependencies in a different PR, this PR should not do any modifications on the .opam file and the dune-project file. Is it clear why?

AryanGodara · 2023-03-21T12:47:56Z

I've updated the new changes,

Thanks! To be a bit clearer on the dependencies: Given that you're fixing the dependencies in a different PR, this PR should not do any modifications on the .opam file and the dune-project file. Is it clear why?

Yes, I understand the reason @pitag-ha !
I'll revert the last commit, and rebase with the master branch, now that the Update-dependencies PR is merged.
Is this correct?

Signed-off-by: aryan <aryangodara03@gmail.com>

pitag-ha · 2023-03-21T13:15:51Z

I'll revert the last commit, and rebase with the master branch, now that the Update-dependencies PR is merged. Is this correct?

Perfect, thank! Yes, that's correct. Once you've gotten rid of the .opam and dune-project file modifications, you probably don't even need to rebase yourself given that there shouldn't be any conflicts and so github should be able do a fast-forward workflow. If you want to go the sure way, rebasing on your side sounds good!

AryanGodara · 2023-03-21T13:18:49Z

I'll revert the last commit, and rebase with the master branch, now that the Update-dependencies PR is merged. Is this correct?

Perfect, thank! Yes, that's correct. Once you've gotten rid of the .opam and dune-project file modifications, you probably don't even need to rebase yourself given that there shouldn't be any conflicts and so github should be able do a fast-forward workflow. If you want to go the sure way, rebasing on your side sounds good!

Yes, I reverted last commit and rebased with origin/main.

Is this PR ready now?

pitag-ha

Thanks for the adaptions, @AryanGodara! You're still modifying the .opam-file. Apart from that LGTM (looks good to me)!

cardio_crumble.opam

Signed-off-by: aryan <aryangodara03@gmail.com>

AryanGodara · 2023-03-21T13:47:57Z

Thanks for the adaptions, @AryanGodara! You're still modifying the .opam-file. Apart from that LGTM (looks good to me)!

Sorry @pitag-ha , removed those two depends

pitag-ha

Perfect, thanks, @AryanGodara!

Btw, one of these days, I'll try to find time to set up a basic CI checking at least whether the code builds and formats.

AryanGodara · 2023-03-21T14:43:12Z

Perfect, thanks, @AryanGodara!

Btw, one of these days, I'll try to find time to set up a basic CI checking at least whether the code builds and formats.

Can I try this @pitag-ha ?
I have a bit of experience with github actions, and CI, and can implement something similar to ocaml-mbr repository CI checks
(basic linting and building test at first, then add to it)

pitag-ha · 2023-03-21T15:04:37Z

Can I try this @pitag-ha ?

If you know how to do that, that sounds great! What kind of CI infrastructure do you have in mind?

You've mentioned ocaml-mbr, which uses the ocaml-ci pipeline. To enroll cardio-crumble to ocaml-ci, I'd have to install the ocaml-ci GitHub app myself. Afterwards, you could go ahead and do the rest: submit the PR to the ocaml-ci stack and add the ocaml-ci status to our README. Given that we don't even have tests for cardio-crumble yet, enrolling it to ocaml-ci could be considered a bit of overhead. However, tbh, to me it sounds good!

Another option could be to use a github actions workflow, making use of ocaml/setup-ocaml@v2.

And I'm also very happy to hear other suggestions if you have something else in mind.

I think in other languages the CI set-up might be more standardized. So, if you were expecting something simpler, don't feel shy to point that out. If you're still happy to work on this, let me know, so that we can open an issue for that and unassign you from #4 for now.

AryanGodara · 2023-03-21T15:15:03Z

Can I try this @pitag-ha ?

If you know how to do that, that sounds great! What kind of CI infrastructure do you have in mind?

You've mentioned ocaml-mbr, which uses the ocaml-ci pipeline. To enroll cardio-crumble to ocaml-ci, I'd have to install the ocaml-ci GitHub app myself. Afterwards, you could go ahead and do the rest: submit the PR to the ocaml-ci stack and add the ocaml-ci status to our README. Given that we don't even have tests for cardio-crumble yet, enrolling it to ocaml-ci could be considered a bit of overhead. However, tbh, to me it sounds good!

Another option could be to use a github actions workflow, making use of ocaml/setup-ocaml@v2.

And I'm also very happy to hear other suggestions if you have something else in mind.

I think in other languages the CI set-up might be more standardized. So, if you were expecting something simpler, don't feel shy to point that out. If you're still happy to work on this, let me know, so that we can open an issue for that and unassign you from #4 for now.

I was originally thinking of github actions, or googling and seeing how it's usually done.

If you install the ocaml-ci , i will research both ways and implement after discussion whichever seems better!

It will take a bit of time though, only till sunday, then my exams will be over and i'll work much faster!

abbysmal reviewed Mar 17, 2023

View reviewed changes

pitag-ha reviewed Mar 17, 2023

View reviewed changes

.ocamlformat Outdated Show resolved Hide resolved

bin/midi.ml Show resolved Hide resolved

bin/midi.ml Show resolved Hide resolved

.gitignore Outdated Show resolved Hide resolved

AryanGodara mentioned this pull request Mar 17, 2023

NOISSUE - Update dune-project dependencies #8

Merged

pitag-ha reviewed Mar 19, 2023

View reviewed changes

bin/midi.mli Show resolved Hide resolved

cardio_crumble.opam Outdated Show resolved Hide resolved

AryanGodara added 5 commits March 21, 2023 18:21

initial commit, add minor scale

a645fa9

Signed-off-by: aryan <aryangodara03@gmail.com>

update .ocamlformat to allow dune build @fmt, added pentatonic scale.

759bd25

Signed-off-by: aryan <aryangodara03@gmail.com>

fix dune build @fmt

f88c1b3

Signed-off-by: aryan <aryangodara03@gmail.com>

remove vscode from gitignore, fix ocamlformat version

9f3cb47

Signed-off-by: aryan <aryangodara03@gmail.com>

add ocamlformat and portmidi

48e5ac3

Signed-off-by: aryan <aryangodara03@gmail.com>

AryanGodara force-pushed the add-scale branch from 41010c5 to 48e5ac3 Compare March 21, 2023 12:51

pitag-ha reviewed Mar 21, 2023

View reviewed changes

cardio_crumble.opam Outdated Show resolved Hide resolved

rem two deps

21c4fc8

Signed-off-by: aryan <aryangodara03@gmail.com>

pitag-ha approved these changes Mar 21, 2023

View reviewed changes

pitag-ha merged commit 5bb33f7 into pitag-ha:main Mar 21, 2023

pitag-ha mentioned this pull request Mar 21, 2023

Add a simple CI #13

Closed

AryanGodara deleted the add-scale branch March 22, 2023 06:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new scale #7

Add a new scale #7

AryanGodara commented Mar 17, 2023

abbysmal Mar 17, 2023

abbysmal Mar 17, 2023 •

edited

Loading

AryanGodara Mar 17, 2023

AryanGodara Mar 17, 2023 •

edited

Loading

abbysmal Mar 17, 2023

pitag-ha Mar 17, 2023

AryanGodara Mar 17, 2023

AryanGodara Mar 17, 2023

pitag-ha Mar 17, 2023

pitag-ha left a comment

pitag-ha left a comment

AryanGodara commented Mar 20, 2023

pitag-ha commented Mar 21, 2023

AryanGodara commented Mar 21, 2023

pitag-ha commented Mar 21, 2023

AryanGodara commented Mar 21, 2023

pitag-ha left a comment

AryanGodara commented Mar 21, 2023

pitag-ha left a comment •

edited

Loading

AryanGodara commented Mar 21, 2023

pitag-ha commented Mar 21, 2023

AryanGodara commented Mar 21, 2023

Add a new scale #7

Add a new scale #7

Conversation

AryanGodara commented Mar 17, 2023

abbysmal Mar 17, 2023

Choose a reason for hiding this comment

abbysmal Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

AryanGodara Mar 17, 2023

Choose a reason for hiding this comment

AryanGodara Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

abbysmal Mar 17, 2023

Choose a reason for hiding this comment

pitag-ha Mar 17, 2023

Choose a reason for hiding this comment

AryanGodara Mar 17, 2023

Choose a reason for hiding this comment

AryanGodara Mar 17, 2023

Choose a reason for hiding this comment

pitag-ha Mar 17, 2023

Choose a reason for hiding this comment

pitag-ha left a comment

Choose a reason for hiding this comment

pitag-ha left a comment

Choose a reason for hiding this comment

AryanGodara commented Mar 20, 2023

pitag-ha commented Mar 21, 2023

AryanGodara commented Mar 21, 2023

pitag-ha commented Mar 21, 2023

AryanGodara commented Mar 21, 2023

pitag-ha left a comment

Choose a reason for hiding this comment

AryanGodara commented Mar 21, 2023

pitag-ha left a comment • edited Loading

Choose a reason for hiding this comment

AryanGodara commented Mar 21, 2023

pitag-ha commented Mar 21, 2023

AryanGodara commented Mar 21, 2023

abbysmal Mar 17, 2023 •

edited

Loading

AryanGodara Mar 17, 2023 •

edited

Loading

pitag-ha left a comment •

edited

Loading