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

Misc tasks to fork #2

Merged
merged 5 commits into from Sep 12, 2021
Merged

Misc tasks to fork #2

merged 5 commits into from Sep 12, 2021

Conversation

cspotcode
Copy link
Collaborator

No description provided.

@LironHazan
Copy link
Collaborator

@cspotcode Hey 👋
I hope it's ok I'm joining the party :)
I forked your branch and hoped to fix the 2 tests that were failing when running with Karma,
The type error TypeError: foo.dynamicMethod is not a function
Is correct, the mockedFoo.dynamicMethod was resolved into MethodToStub which is an internal class instantiated from createMethodToStub(), I didn't give it a deeper look yet.
With Jest we're simply getting the proxy and the dynamicMethod is of type function,
Did you checked if those specific tests are passing in the origin repo?

@cspotcode
Copy link
Collaborator Author

@LironHazan good idea, I don't remember, but I do not think I checked if they were passing in the origin repo.

If you would like to be a collaborator on this repo, let me know and I can add you. It will give you permission to push to this branch directly.

@LironHazan
Copy link
Collaborator

@cspotcode thanks, I'll be happy to collaborate, btw in future thinking of modern stack, would we want to use eslint instead of tslint? (TSLint has been deprecated in favor of ESLint. Please see palantir/tslint#4534 for more information.)

@cspotcode
Copy link
Collaborator Author

eslint sounds good to me. I'm also happy using no linter, only prettier and tsc. Either is fine.

I have added you as a collaborator so you should have access to triage issues and pull requests and push code.

@LironHazan
Copy link
Collaborator

prettier is great, I tend to combine both :) Thanks!

@LironHazan
Copy link
Collaborator

@cspotcode I've ran the tests in origin branch they passed so I've noticed following commit:
NagRock@3e51222
which isn't part of this fork,
I grabbed the diffs and all is green now!

TOTAL: 233 SUCCESS

Finished in 0.729 secs / 0.239 secs @ 21:05:33 GMT+0300 (Israel Daylight Time)

SUMMARY:
✔ 233 tests completed

pushing changes

@LironHazan
Copy link
Collaborator

@cspotcode sorry to nudge, not sure what happened, the git actions weren't on the fork-tasks local branch so I got them back from upstream/master, I don't see it hooked on commit at the moment

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@6637048). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master       #2   +/-   ##
=========================================
  Coverage          ?   93.41%           
=========================================
  Files             ?       34           
  Lines             ?      653           
  Branches          ?       80           
=========================================
  Hits              ?      610           
  Misses            ?       30           
  Partials          ?       13           

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 6637048...9717aa0. Read the comment docs.

@cspotcode
Copy link
Collaborator Author

Awesome, if you think this pull request is ready to merge, go for it.

@LironHazan
Copy link
Collaborator

@cspotcode sure so I'll merge and will branch out to a "tasks-3" branch which will contain some new configs, wdyt?

@LironHazan LironHazan merged commit 25b8f1d into master Sep 12, 2021
@cspotcode
Copy link
Collaborator Author

cspotcode commented Sep 12, 2021

I think we can name the branches according to the changes in them, like eslint-prettier if that is what you're changing in that branch.

We may want to postpone large stylistic changes if they make it more difficult to merge existing pull requests from the NagRock and johanblumenberg repositories.

@LironHazan
Copy link
Collaborator

eslint-prettier sounds good, but sure let's have it on a later phase

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.

None yet

3 participants