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.

Sign up for GitHub

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

Jump to bottom

fix(upgrade-task): Fixed upgrade task to include line ending in package.json #1650

Merged
merged 3 commits into from
Mar 4, 2022
Merged

fix(upgrade-task): Fixed upgrade task to include line ending in package.json #1650

merged 3 commits into from
Mar 4, 2022

Conversation

AminFazlMondo
Copy link
Contributor

If the package manager is set to npm, the npm install command adds a new line to the end of package.json. This is a known issue: npm/npm#18545
All of the other tasks do not do this currently, and this causes the upgrade workflow to fail for the first time and self mutation fixes that. To avoid that, added this new line to package.json anyway.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@AminFazlMondo
Copy link
Contributor Author

I would like to get some feedback on this before investing too much.
Is this approach in line with the vision of the repo?
With the failures in the tests, I was wondering if it is OK to update the test, or take other approaches.

@Chriscbr
Copy link
Contributor

Chriscbr commented Mar 1, 2022

I think this is ok if it fixes issues / makes a more natural experience when the package manager is set to npm. If the package manager is set to yarn, then I think the output should be left as is, without an extra newline.

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2022 •
edited
Loading

Codecov Report

Merging #1650 (0b2d244) into main (d90284c) will increase coverage by 1.41%.
The diff coverage is 89.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1650      +/-   ##
==========================================
+ Coverage   88.06%   89.47%   +1.41%     
==========================================
  Files         132      150      +18     
  Lines        5109     6054     +945     
  Branches     1207     1555     +348     
==========================================
+ Hits         4499     5417     +918     
- Misses        610      635      +25     
- Partials        0        2       +2
Impacted Files Coverage Δ
src/awscdk/internal.ts 100.00% <ø> (ø)
src/cdk/consts.ts 100.00% <ø> (+36.36%) ⬆️
src/dev-env.ts 83.33% <0.00%> (ø)
src/java/index.ts 100.00% <ø> (ø)
src/python/index.ts 100.00% <ø> (ø)
src/release/index.ts 100.00% <ø> (ø)
src/testing.ts 80.00% <ø> (ø)
src/textfile.ts 100.00% <ø> (ø)
src/toml.ts 90.90% <ø> (ø)
src/typescript/index.ts 100.00% <ø> (ø)
... and 178 more

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 90f5546...0b2d244. Read the comment docs.

@AminFazlMondo
Copy link
Contributor Author

Hi @Chriscbr,
Could you please have a look at this again?
Cheers,

@mergify mergify bot merged commit 184b4e2 into projen:main Mar 4, 2022
@AminFazlMondo AminFazlMondo deleted the fix/UpgradeLineEndings branch May 20, 2022 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chriscbr Chriscbr Chriscbr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AminFazlMondo @Chriscbr @codecov-commenter