Skip to content

Check if startsWith exists before setting it #14

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
Merged
merged 5 commits into from
Mar 25, 2021
Merged

Check if startsWith exists before setting it #14

merged 5 commits into from
Mar 25, 2021

Conversation

karthiknadig
Copy link
Contributor

Closes #13

huan
huan approved these changes Mar 24, 2021
View reviewed changes
Copy link
Collaborator

@huan huan left a comment

Choose a reason for hiding this comment

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

LGTM

@huan
Copy link
Collaborator

huan commented Mar 24, 2021

@karthiknadig Thank you very much for the improvement!

However, there's some CI failed. I know it's not related to this PR, but in order to make all the CI green to this PR, could you please fix them as well? Appreciate it.

@karthiknadig
Copy link
Contributor Author

@huan Looks like nyc had to be updated. But the latest version of nyc does not work on node 6 and below. Additionally, I am explicitly skipping the startsWith bits from coverage, since it is not needed any more.

@huan huan merged commit 5ebe846 into simonepri:master Mar 25, 2021
@huan
Copy link
Collaborator

huan commented Mar 25, 2021

Thank your very much, merged!

@karthiknadig
Copy link
Contributor Author

@huan Any idea when this will be published?

@huan huan mentioned this pull request Mar 25, 2021
Closed
@huan
Copy link
Collaborator

huan commented Mar 25, 2021

I have not checked if our CI can publish the latest code automatically. If it's not, then the publish will depend on how the release strategy is defined by @simonepri

@simonepri
Copy link
Owner

Hi there, sorry for being late and thanks for contributing!

About the CI, I think it was discussed already but the idea is that we need to do a new major release before removing node 4 etc to fix the ci.

So in order for me to release this asap my idea would be to revert all changes but the fix itself.
What do you think?

@karthiknadig
Copy link
Contributor Author

@simonepri Works for me.

karthiknadig added a commit to karthiknadig/pidtree that referenced this pull request Mar 26, 2021
@karthiknadig
Revert "Check if startsWith exists before setting it (simonepri#14)"
42e9e44 
This reverts commit 5ebe846.
@karthiknadig karthiknadig mentioned this pull request Mar 26, 2021
Closed
simonepri added a commit that referenced this pull request Jun 4, 2022
@simonepri @karthiknadig
Remove startsWith redefinition for node >= 4.0.0
cafbb74 
Closes #14 #15

Co-Authored-By: Karthik Nadig <[email protected]>
simonepri added a commit that referenced this pull request Jun 4, 2022
@simonepri @karthiknadig
Remove startsWith redefinition for node >= 4.0.0
a0f2877 
Closes #14 #15

Co-Authored-By: Karthik Nadig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huan huan huan 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.

startsWith doesn't work with the two argument form.
3 participants
@karthiknadig @huan @simonepri