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

Assorted link fixes #3671

Merged
merged 3 commits into from
Apr 6, 2021
Merged

Assorted link fixes #3671

merged 3 commits into from
Apr 6, 2021

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Apr 6, 2021

There are plenty more redirected links, but I thought I'd make a first PR with these fixes. :)

I could probably squash the first and the third patch into one if you prefer.

PS1. I'd appreciate if someone could update the build assets for me before merging this. I thought this was automated and there are too many npm vulnerabilities AFAICT to even try to do npm i.
PS2. I'm not familiar with the commitlint conventions, so bear with me here
PS3. I personally prefer using Number.POSITIVE_INFINITY for concurrency, but I'm not 100% sure on which Node.js versions is supported. It doesn't change the functionality but it's more explicit.

@AppVeyorBot
Copy link

Build karma 2969 completed (commit 48ad6b8d51 by @XhmikosR)

@karmarunnerbot
Copy link
Member

Build karma 571 completed (commit 48ad6b8d51 by @XhmikosR)

@karmarunnerbot
Copy link
Member

Build karma 572 completed (commit 48ad6b8d51 by @XhmikosR)

@devoto13
Copy link
Collaborator

devoto13 commented Apr 6, 2021

  1. It is automated, the generated website will be updated once the next release is published. So you don't need to worry about it.

  2. You can do something like the below to make CI happy:

    docs: fix redirected npmjs links
    docs: change npm to lowercase
    docs: use https when possible

  3. I would say, leave it be.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Apr 6, 2021

Thanks for the hints! That being said, the changes don't affect only the docs. How can I combine more than one component? Maybe docs,core?

@devoto13
Copy link
Collaborator

devoto13 commented Apr 6, 2021

AFAICT only comments are updated, so it should be fine with just docs. Or you can use “docs(core): message” if changes are specific to core.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Apr 6, 2021

There's one link in code too but I don't think it matters a lot.

@XhmikosR XhmikosR marked this pull request as ready for review April 6, 2021 13:51
@AppVeyorBot
Copy link

Build karma 2970 completed (commit e6eeb58478 by @XhmikosR)

@karmarunnerbot
Copy link
Member

Build karma 573 completed (commit e6eeb58478 by @XhmikosR)

@karmarunnerbot
Copy link
Member

Build karma 572 completed (commit e6eeb58478 by @XhmikosR)

@devoto13
Copy link
Collaborator

devoto13 commented Apr 6, 2021

There's one link in code too but I don't think it matters a lot.

Yes.

PS I've amended the last commit to make CI happy. These were changes in the generated files coming from node_modules, so we can't change them to https here.

@karmarunnerbot
Copy link
Member

Build karma 574 completed (commit 9f4779f7ee by @XhmikosR)

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Apr 6, 2021

I'd like to keep those links, though. Can't you just update the dist files?

@AppVeyorBot
Copy link

Build karma 2971 completed (commit 9f4779f7ee by @XhmikosR)

@karmarunnerbot
Copy link
Member

Build karma 573 completed (commit 9f4779f7ee by @XhmikosR)

@devoto13
Copy link
Collaborator

devoto13 commented Apr 6, 2021

I'd like to keep those links, though. Can't you just update the dist files?

Unfortunately, we can't do it. Otherwise, we'll have to give up (unnecessarily complicate) a check that bundled client code is up-to-date, which is IMO more important than not having HTTP links.

The right fix would be to change the link in https://github.com/webmodules/dom-serialize, get a release, and update, but the package looks abandoned. Not sure if it is worth the trouble.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Apr 6, 2021

Ah, OK, that makes sense. I wasn't aware that was 3rd-party code. All good then :)

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Apr 6, 2021

BTW the bots are too chatty 😛

@devoto13 devoto13 requested a review from johnjbarton April 6, 2021 14:18
@devoto13
Copy link
Collaborator

devoto13 commented Apr 6, 2021

BTW the bots are too chatty 😛

You can block them to avoid any notifications. At least that's what I did 😄

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

Thanks!

@johnjbarton johnjbarton merged commit 5176aff into karma-runner:master Apr 6, 2021
@XhmikosR XhmikosR deleted the links branch April 6, 2021 18:37
@karmarunnerbot
Copy link
Member

🎉 This PR is included in version 6.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
*  fix redirected npmjs links
* change npm to lowercase
* use https when possible
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants