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

Infer polyA tail & Conda to Mamba #102

Merged
merged 2 commits into from
Dec 5, 2022
Merged

Infer polyA tail & Conda to Mamba #102

merged 2 commits into from
Dec 5, 2022

Conversation

BorisYourich
Copy link
Collaborator

@BorisYourich BorisYourich commented Nov 28, 2022

Description

Fixes #86
Fixes #101

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality
    to not work as expected)
  • Documentation update

Checklist

Please carefully read these items and tick them off if the statements are true
or do not apply.

  • I have performed a self-review of my own code
  • My code follows the existing coding style, lints and generates no new
    warnings
  • I have added type annotations to all function/method signatures, and I
    have added type annotations for any local variables that are non-trivial,
    potentially ambiguous or might otherwise benefit from explicit typing.
  • I have commented my code in hard-to-understand areas
  • I have added ["Google-style docstrings"] to all new modules, classes,
    methods/functions or updated previously existing ones
  • I have added tests that prove my fix is effective or that my feature
    works
  • New and existing unit tests pass locally with my changes and I have not
    reduced the code coverage relative to the previous state
  • I have updated any sections of the app's documentation that are affected
    by the proposed changes

If for some reason you are unable to tick off all boxes, please leave a
comment explaining the issue you are facing so that we can work on it
together.

@uniqueg uniqueg changed the title ci: switched to mamba from conda in all jobs build: use Mamba instead of Conda Dec 3, 2022
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

EDIT: Okay, I read the PR description :)

EDIT2: But PR #99 is closed, now I'm confused. In what order should PRs be merged? It would make sense to me to start with this one (after removing the polyA-related stuff).

Mamba changes look fine, but the PR still contains polyA changes. Is it supposed to be merged after the PR with those changes?

@BorisYourich
Copy link
Collaborator Author

EDIT: Okay, I read the PR description :)

EDIT2: But PR #99 is closed, now I'm confused. In what order should PRs be merged? It would make sense to me to start with this one (after removing the polyA-related stuff).

Mamba changes look fine, but the PR still contains polyA changes. Is it supposed to be merged after the PR with those changes?

Not necessarily, it was intended in that order, but the previous PR contains just the subset of changes of this one. This branch was created from the polyA_tails branch so that when the polyA_tails is merged this one does not have any conflicts, but since the polyA is closed, you might as well just merge this one, which has both the CI as well as polyA changes.

@uniqueg
Copy link
Member

uniqueg commented Dec 5, 2022

Yes, but that is the problem: the PR title and description does not reflect that! And so if I squash merge, there will be no entry in the history that will mention or describe the polyA tail code. And if I don't squash, we will get 7 commits for partial implementations, including a revert. What we want instead is two commits, one for the Mamba vs Conda change, the other for the polyA tail functionality. Two options here:

  1. You do git rebase --interactive on this branch and clean it up such that there are 2 properly named conventional commits (one build: use Mamba instead of Conda and one feat: infer presence of polyA tail).
  2. You create two PRs from two separate feature branches, one addressing the Conda/Mamba change, the other the polyA tail change.

The second option is generally preferred because I can review independently. But in this case I'm fine with either.

@BorisYourich
Copy link
Collaborator Author

Yeah sure, I did not expect you would close the first one :D, will keep this in mind if I ever make two or more consecutive PRs in the future. I will go with rebase just to keep it simple for now.

@uniqueg
Copy link
Member

uniqueg commented Dec 5, 2022

Oh, my bad, I misunderstood. Could have just reopened #99 then.

@uniqueg uniqueg merged commit 24d3702 into dev Dec 5, 2022
@uniqueg uniqueg deleted the mamba_workflows branch December 5, 2022 16:17
@uniqueg uniqueg changed the title build: use Mamba instead of Conda Infer polyA tail & Conda to Mamba Dec 5, 2022
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.

Change from conda to mamba in CI workflows Infer presence of polyA tails in reads
2 participants