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

fix(manifest): do not group changes under "." #841

Merged
merged 1 commit into from
Mar 25, 2021
Merged

fix(manifest): do not group changes under "." #841

merged 1 commit into from
Mar 25, 2021

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Mar 25, 2021

If "." was anything but the last path in the list, it would result in only receiving changes for the special "." path.

@bcoe bcoe requested a review from a team March 25, 2021 20:34
@bcoe bcoe requested a review from a team as a code owner March 25, 2021 20:34
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 25, 2021
@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #841 (e22de92) into master (183b235) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #841   +/-   ##
=======================================
  Coverage   93.74%   93.74%           
=======================================
  Files          64       64           
  Lines        9156     9160    +4     
  Branches      930      983   +53     
=======================================
+ Hits         8583     8587    +4     
  Misses        570      570           
  Partials        3        3           
Impacted Files Coverage Δ
src/commit-split.ts 100.00% <100.00%> (ø)

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 183b235...e22de92. Read the comment docs.

@bcoe bcoe merged commit 47b8b43 into master Mar 25, 2021
@bcoe bcoe deleted the fix-dot branch March 25, 2021 20:42
pkgName = this.packagePaths.find(p => {
// The special "." path, representing the root of the module
// should be ignored by commit-split:
return file.indexOf(p) >= 0 && p !== '.';
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, it seems like the '.' packagePath will never get any commits unless I'm reading this wrong? how will it get changelog entries and version bumps?

Copy link
Collaborator

Choose a reason for hiding this comment

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

but it seems to work. I confess I don't feel like I totally understand really how we're assigning commits to packages. I have a case were a commit got assigned to a package but none of the files for the commit are in the package path... and now that I wrote that I know exactly what happened in my case:

package path: "python"
commit files all under "examples/python/..."
but "examples/python/foo.py".indexOf("python") >= 0 is true! I think we want a stricter check here? probably

return file.indexOf(p) === 0 && p !== '.';

I'll submit a patch for this bug, but I still don't understand how the "." package is being assign the appropriate commits as it seems to be...

Copy link
Collaborator

Choose a reason for hiding this comment

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

the '.' packagePath will never get any commits unless I'm reading this wrong?

ok, I remember now: you're giving the '.' package its commits in manifest.ts rather than in here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants