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

BREAKING CHANGE: chore: Migrate to ferrum #132

Merged
merged 1 commit into from
Jul 16, 2019
Merged

BREAKING CHANGE: chore: Migrate to ferrum #132

merged 1 commit into from
Jul 16, 2019

Conversation

koraa
Copy link
Contributor

@koraa koraa commented Jul 8, 2019

No description provided.

@ghost
Copy link

ghost commented Jul 8, 2019

There were the following issues with this Pull Request

  • Commit: b46dd4a
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@koraa
Copy link
Contributor Author

koraa commented Jul 8, 2019 •
edited
Loading

Will be merged tomorrow around noon CEST

@koraa koraa requested a review from trieloff July 8, 2019 08:59
@koraa koraa force-pushed the karo/ferrum branch 3 times, most recently from d8e97e0 to 1fe3cfa Compare July 8, 2019 09:32
@codecov
Copy link

codecov bot commented Jul 8, 2019 •
edited
Loading

Codecov Report

Merging #132 into master will decrease coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage      99%   98.74%   -0.27%     
==========================================
  Files          19       15       -4     
  Lines        1513     1037     -476     
  Branches      324      253      -71     
==========================================
- Hits         1498     1024     -474     
+ Misses         15       13       -2
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/string.js 100% <100%> (ø) ⬆️
src/HelixConfig.js 90.81% <100%> (-0.1%) ⬇️
src/dom.js 98.87% <100%> (-0.02%) ⬇️

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 d954f3a...1b592e8. Read the comment docs.

@stefan-guggisberg
Copy link
Contributor

Will be merged tomorrow around noon CEST

Please don't merge unless reviewed by @trieloff

There's also 1 test failing.

@koraa
Copy link
Contributor Author

koraa commented Jul 8, 2019

The smoke tests are failing because the related PRs are not merged yet.
@stefan-guggisberg Is there any specific objection you have to merging this PR?

@stefan-guggisberg
Copy link
Contributor

@stefan-guggisberg Is there any specific objection you have to merging this PR?

It's a large PR and I am just bothered that you declared to merge it tomorrow although there's still a review pending.

@koraa
Copy link
Contributor Author

koraa commented Jul 8, 2019

I suggest you take another look; it really just moves all the functions into a separate repo and that repo has been well reviewed :)
I like to avoid having PRs linger for a long time, especially now that a lot of people are on holidays; as far as I know @trieloff is not extremely available right now and I think waiting for two weeks to merge a small refactoring is a bad idea – of course if you have specific objections or need more time to review the code I am happy to wait a day or two more before I merge :)
@stefan-guggisberg How about you – or @tripodsan take a quick look at the three prs, and then we're good to go :)

trieloff
trieloff approved these changes Jul 8, 2019
View reviewed changes
Copy link
Contributor

@trieloff trieloff left a comment

Choose a reason for hiding this comment

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

Looks safe.

@stefan-guggisberg stefan-guggisberg removed their request for review July 8, 2019 19:31
tripodsan
tripodsan requested changes Jul 9, 2019
View reviewed changes
Copy link
Contributor

@tripodsan tripodsan left a comment

Choose a reason for hiding this comment

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

I would like to do this in 2 steps:

  1. deprecate the existing exports. delete the own implementation but keep exporting the ferrum fuctions. maybe proxy behind a getter that uses https://nodejs.org/docs/latest-v10.x/api/process.html#process_process_emitwarning_warning_options
    1.5 then we have time to adjust all other modules to use ferrum
  2. remove the deprecated exports.

@koraa koraa closed this Jul 9, 2019
@koraa koraa reopened this Jul 9, 2019
@koraa
Copy link
Contributor Author

koraa commented Jul 9, 2019

Triggering smoke tests again…

@koraa
Copy link
Contributor Author

koraa commented Jul 9, 2019

@trieloff Thanks for the quick review :)

@koraa
Copy link
Contributor Author

koraa commented Jul 9, 2019

@tripodsan The dependents should have been updated with the other PRs; are there any ones besides in helix-pipeline and helix-cli?

@koraa koraa closed this Jul 10, 2019
@koraa koraa reopened this Jul 10, 2019
@koraa
Copy link
Contributor Author

koraa commented Jul 10, 2019

Hmm, @kptdobe which version of helix-pipeline the smoke tests be using? Seems they are stuck on an older version which has not been ported to ferrum?

@kptdobe
Copy link
Contributor

kptdobe commented Jul 15, 2019

Sorry, missed your comment.
The smoke tests builds a version of CLI from master and simply patches all dependencies with the helix-shared from your PR.
The corresponding job is https://circleci.com/gh/adobe/helix-continuous/3647 and at the end of the "Install using git branches (master or specified)" step, you see the dependency tree:

@adobe/helix-cli@4.9.1 /home/circleci/project/helix-cli
├── @adobe/fastly-native-promises@1.10.0 
├─┬ @adobe/helix-pipeline@4.0.0 
│ ├── @adobe/helix-shared@1.5.1  deduped (github:adobe/helix-shared#1630c115db0e23a2aa59a5982d781c18529d6142)
│ └── @adobe/openwhisk-loggly-wrapper@0.4.3 
├── @adobe/helix-shared@1.5.1  (github:adobe/helix-shared#1630c115db0e23a2aa59a5982d781c18529d6142)
├─┬ @adobe/helix-simulator@2.12.13 
│ ├── @adobe/git-server@0.9.17 
│ └── @adobe/helix-shared@1.5.1  -> /home/circleci/project/helix-cli/node_modules/@adobe/helix-shared (github:adobe/helix-shared#1630c115db0e23a2aa59a5982d781c18529d6142)
├── @adobe/helix-testutils@0.2.0 
└── @adobe/htlengine@3.2.0

i.e: @adobe/helix-pipeline@4.0.0 :)

@koraa @tripodsan
refactor(ferrum): Migrate to ferrum
1b592e8 
fixes #124

BREAKING CHANGE: the following modules are no longer exports: functional, op, types, sequence
@tripodsan tripodsan merged commit cc64bb6 into master Jul 16, 2019
@tripodsan tripodsan deleted the karo/ferrum branch July 16, 2019 05:50
adobe-bot pushed a commit that referenced this pull request Jul 16, 2019
@semantic-release-bot
chore(release): 2.0.0 [skip ci]
928a3d2 
# [2.0.0](v1.5.1...v2.0.0) (2019-07-16)

### Code Refactoring

* **ferrum:** Migrate to ferrum ([#132](#132)) ([cc64bb6](cc64bb6)), closes [#124](#124)

### BREAKING CHANGES

* **ferrum:** the following modules are no longer exports: functional, op, types, sequence
@adobe-bot
Copy link

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tripodsan tripodsan tripodsan approved these changes

@trieloff trieloff trieloff approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@koraa @stefan-guggisberg @kptdobe @adobe-bot @trieloff @tripodsan