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

Retrieve link name from link when using link asset #401

Closed
wants to merge 7 commits into from

Conversation

awcjack
Copy link
Contributor

@awcjack awcjack commented Jun 11, 2022

link name is required for link asset following documentation
If label property is not provided, try to retrieve the link name from url

Close #393

if (!label) {
const parsedUrl = new URL(_url);
const file = pathlib.basename(parsedUrl.pathname);
_label = file ? file : 'Unknown asset';
Copy link
Contributor

@fgreinacher fgreinacher Jun 14, 2022

Choose a reason for hiding this comment

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

I don't like the fallback here. What about using parsedUrl.hostname?

Also can any of these throw an exception that we should handle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean replacing 'Unknown asset' by parsedUrl.hostname?
Actually I am thinking any better handling here too since the link name must be unique within the release.

So when user have urls like below will cause exception during publish
https://server1.com/1.txt
https://server2.com/1.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

We could include the index from .map, e.g.:

_label = file ? file : `${parsedUrl.hostname}_${index}`;

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@fgreinacher fgreinacher left a comment

Choose a reason for hiding this comment

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

I like this @awcjack! Left some comments.

@fgreinacher
Copy link
Contributor

@awcjack Are you planning to continue here? If not just speak and I can finish it!

awcjack and others added 2 commits June 23, 2022 17:11
Co-authored-by: Florian Greinacher <[email protected]>
Co-authored-by: Florian Greinacher <[email protected]>
@awcjack
Copy link
Contributor Author

awcjack commented Jun 23, 2022

@awcjack Are you planning to continue here? If not just speak and I can finish it!

Quite busy last week and forget the pull request 🙈
Sorry for the delay

@fgreinacher
Copy link
Contributor

This PR has been stale for over a year now, so I'm closing it.

Feel free to recreate if needed :)

@fgreinacher fgreinacher closed this Oct 5, 2023
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.

Failing (?) feature "allow adding URLs to release assets"
3 participants