fmalmeida / bacannot

Generic but comprehensive pipeline for prokaryotic genome annotation and interrogation with interactive reports and shiny app.
https://bacannot.readthedocs.io/en/latest/
GNU General Public License v3.0
98 stars 9 forks source link

Change case of modules/workflows to distinguish them from parameters #38

Closed abhi18av closed 2 years ago

abhi18av commented 3 years ago

The current naming convention makes it a bit dubious to determine the exact nature of an identifier, for example in the snippet below

      call_methylation(prokka.out[6])
      methylation_out_1 = call_methylation.out[2]
      methylation_out_2 = call_methylation.out[3]

Generally speaking, with DSL2, the community has adopted the paradigm of using MODULE_NAME / WORKFLOW_NAME pattern and regarding channels a preference is use a suffix like ch. Therefore the above snippet would be like

      CALL_METHYLATION(PROKKA.out[6])
      methylation_out_1_ch = CALL_METHYLATION.out[2]
      methylation_out_2_ch = CALL_METHYLATION.out[3]

What are your thoughts?

fmalmeida commented 3 years ago

Yes, I agree that is better to distinguish modules/workflows to distinguish them from parameters. The pipeline would gain in readability, and standardization.

Please, also see my comments in #39 since it is related to this one.

fmalmeida commented 2 years ago

Hi @abhi18av,

I have started to make these changes while trying to remodel the pipeline in branch https://github.com/fmalmeida/bacannot/tree/remodeling, related to issue #36 ... So we could try to address this issue in the same branch?

Or it is better to have a specific branch and specific PR for this issue and also for issue #39 ?

abhi18av commented 2 years ago

I think it's always better to have smaller PRs, this way if something goes wrong we know exactly where do we need to rollback.

Preferably, a PR address a single issue.

fmalmeida commented 2 years ago

Sure thing! Let's keep this way them!

Therefore, I will stash the changes I have made on that branch about the case of modules/workflows and let this issue (#38) here address this, while the remodeling branch (created to resolve issue #36) will only try to address the usage of <conda/docker/singularity> and the required databases.

About the develop branch in which we had already merged your PR, do you believe its best to wait for issues #39 and this one to be addressed prior to pushing develop to master?

abhi18av commented 2 years ago

I think that the changes introduced by https://github.com/fmalmeida/bacannot/pull/43 should not create big difference in the pipeline and once it has been tested on your end we can merge it on the master.

However, from now onwards, I am planning to follow this pattern

  1. Fork abhinav/feature from develop
  2. Do my changes in abhinav/feature and create a PR against develop
  3. Once they are merged (🤞) into develop we do further end testing
  4. Then we create a release using a PR from develop to master.

Hopefully, this we can have visibility on what we are working on and have minimal merge conflicts.

Sounds good?

fmalmeida commented 2 years ago

Ok!

I will make the tests then in the develop branch to check if it could be merged.

And I totally agree with the plan you outlined. I will also try to make sure the develop branch is up-to-date with master to avoid conflicts when going to step 4.

abhi18av commented 2 years ago

Hey @fmalmeida , circling back for this one - let me know if there's anything I can help with :)

fmalmeida commented 2 years ago

Hi @abhi18av,

While trying to solve issue #36, I already started to change the casenames of channels and modules (which is related to the present issue). However, the issue #36 itself may be a little bit slow to come, but I am already changing it :)

Maybe it will be best to address this all there?

Or you believe it would be nice to solve this issue by itself, and then, when it's PR is merged to the develop I then try to bring its changes forward to the branch where issue #36 is being tackled?

Genuinely asking :) Don't know which one would be simpler or easier to manage and organise.

abhi18av commented 2 years ago

I'm of the thought that a PR should address a single issue and should be small in size.

I noticed that there's a WIP branch https://github.com/fmalmeida/bacannot/tree/remodeling - could you please create a draft PR in the repo itself, it'd help in understanding the exact changes.

Beyond this, just to confirm you're in agreement with https://github.com/fmalmeida/bacannot/issues/38#issue-1055147985 right?

fmalmeida commented 2 years ago

Okay then. Let's make it that way:

  1. We solve this issue here, by itself.
  2. I will create a draft PR to keep track of changes related to issue #36 and its branch "remodeling"
  3. I will stop making changes related to casename in issue #36 so it is easier to solve conflicts when bringing the changes of this issue here to the #36
  4. Finally, when this issue is merged and its PR accepted I bring its changes to issue #36 so when it is finished, it does not raise too much conflicts with the changes found in the merged develop.

How does it sounds? Sounds ok?

And yes, I agree with the comments on changing the casenames to avoid confusion.

:)

abhi18av commented 2 years ago

Sounds good to me @fmalmeida - thanks 😊

fmalmeida commented 2 years ago

This has been addressed by PR #45

Thanks for the help @abhi18av

abhi18av commented 2 years ago

You're welcome Felipe! 😊