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

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

Closed
abhi18av opened this issue Nov 16, 2021 · 13 comments · Fixed by #45
Closed

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

abhi18av opened this issue Nov 16, 2021 · 13 comments · Fixed by #45

Comments

@abhi18av
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Owner

fmalmeida commented Nov 23, 2021

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
Copy link
Contributor Author

abhi18av commented Nov 23, 2021

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
Copy link
Owner

fmalmeida commented Nov 23, 2021

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
Copy link
Contributor Author

I think that the changes introduced by #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
Copy link
Owner

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
Copy link
Contributor Author

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

@fmalmeida
Copy link
Owner

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
Copy link
Contributor Author

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 #38 (comment) right?

@fmalmeida
Copy link
Owner

fmalmeida commented Dec 17, 2021

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 re-think modules organization and structure #36 and its branch "remodeling"
  3. I will stop making changes related to casename in issue re-think modules organization and structure #36 so it is easier to solve conflicts when bringing the changes of this issue here to the re-think modules organization and structure #36
  4. Finally, when this issue is merged and its PR accepted I bring its changes to issue re-think modules organization and structure #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
Copy link
Contributor Author

Sounds good to me @fmalmeida - thanks 😊

@fmalmeida
Copy link
Owner

This has been addressed by PR #45

Thanks for the help @abhi18av

@fmalmeida fmalmeida linked a pull request Jan 6, 2022 that will close this issue
@abhi18av
Copy link
Contributor Author

You're welcome Felipe! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants