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

WIP - feat: filter for lerna tags in independent mode #222

Closed

Conversation

StarpTech
Copy link
Contributor

@StarpTech StarpTech commented Jun 13, 2022

Fix #220

Description

Motivation and Context

How Has This Been Tested?

Types of changes

  • 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 change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #222 (c80cf89) into main (bbc9934) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #222      +/-   ##
==========================================
+ Coverage   93.25%   93.26%   +0.01%     
==========================================
  Files         134      134              
  Lines        3968     3987      +19     
  Branches      804      812       +8     
==========================================
+ Hits         3700     3718      +18     
- Misses        268      269       +1     
Impacted Files Coverage Δ
.../core/src/utils/collect-updates/collect-updates.ts 98.00% <100.00%> (+0.18%) ⬆️
...ges/core/src/utils/collect-updates/lib/has-tags.ts 100.00% <100.00%> (ø)
packages/core/src/utils/describe-ref.ts 100.00% <100.00%> (ø)
packages/exec/src/exec-command.ts 98.92% <100.00%> (+0.02%) ⬆️
packages/list/src/list-command.ts 100.00% <100.00%> (ø)
packages/publish/src/publish-command.ts 96.42% <100.00%> (ø)
packages/run/src/run-command.ts 95.91% <100.00%> (+0.03%) ⬆️
packages/version/src/version-command.ts 99.73% <100.00%> (+0.01%) ⬆️
packages/core/src/package.ts 99.38% <0.00%> (-0.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c645aef...c80cf89. Read the comment docs.

@@ -1,5 +1,5 @@
import log from 'npmlog';
import { ExecOpts, UpdateCollectorOptions } from '../../models';
import {DescribeRefOptions, ExecOpts, UpdateCollectorOptions} from '../../models';
Copy link
Member

Choose a reason for hiding this comment

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

are you using VSCode? I would assume that Prettier would add spaces around curly braces here, that is if you have the Prettier VSCode extension installed, if not please add spaces on both end of import


const graph = buildGraph();
const pkgs = graph.rawPackageList;
const execOpts = { cwd: "/test", match: '*@*'};
Copy link
Member

Choose a reason for hiding this comment

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

can you provide an example of a tag with this pattern match *@*? I mean what would a typical tag look like for this? Might be better to add a unit test with such example!?

Copy link
Contributor Author

@StarpTech StarpTech Jun 14, 2022

Choose a reason for hiding this comment

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

All tags tagged by lerna follow @wundergraph/sdk@0.92.6 in independent mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be better to add a unit test with such example!?

Yes, let me check.

@ghiscoding
Copy link
Member

ghiscoding commented Jun 17, 2022

@StarpTech hello, I'm back home, are you expecting to update the PR with what was identified in the review? or should I publish a patch release with the other changes you were waiting for?

Also in the description of this PR, you mentioned that it fixes #200 but I think you meant that it fixes #220 instead, isn't it?

@ghiscoding
Copy link
Member

@StarpTech hi there, not sure if you saw my last comment but I'm basically waiting for code change and feedback from your side to go further

@StarpTech
Copy link
Contributor Author

StarpTech commented Jun 19, 2022

Hi @ghiscoding, I'm going to address the feedback in the next days.

@StarpTech
Copy link
Contributor Author

Quick update, it will take a bit longer.

@ghiscoding
Copy link
Member

ghiscoding commented Jun 28, 2022

@StarpTech do you have a possible ETA to finish this PR fix? If it's within a week then I can wait, or else I can merge all my other PRs and make a new release without your PR but by doing it might (possibly will) bring possible conflicts in your PR which I was trying to avoid.

EDIT

adding WIP prefix so I don't merge it accidently

@ghiscoding ghiscoding changed the title feat: filter for lerna tags in independent mode WIP - feat: filter for lerna tags in independent mode Jun 30, 2022
@ghiscoding
Copy link
Member

@StarpTech
What is missing in this PR apart from unit test? Can it be merged as it is? Anything I can help to get this PR merge? I'd really like this to be merged shortly

@ghiscoding
Copy link
Member

ghiscoding commented Jul 20, 2022

Superseded by PR #267 which got merged (by assuming that this PR was ok as it was originally created), since no feedback was provided for the past few weeks

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.

Complete - lerna version gets confused by non-lerna tags
2 participants