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

Use startsWith and endsWith when possible. #429

Closed
wants to merge 4 commits into from

Conversation

XhmikosR
Copy link
Collaborator

@XhmikosR XhmikosR commented Nov 13, 2019

Makes the intention clearer.

TODO:

  • figure out how to handle the Vinyl test failure

src/file.js Outdated
@@ -35,11 +35,15 @@ function normalizePath(str) {

/**
* Check whether a resource is external or not
* @param {string} href Path
* @param {string|Vinyl} href Path
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not super familiar with jsdoc so please confirm this is right

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't be ;)
Do you have a hint to start debugging where the Vinyl object is coming from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It happens in one of the vinyl tests. I'll revert this part so that we get more info.

src/file.js Outdated
* @returns {boolean} True if the path is remote
*/
function isRemote(href) {
return /(^\/\/)|(:\/\/)/.test(href) && !href.startsWith('file:');
if (isVinyl(href)) {
return false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ended up adding a separate check for Vinyl. I'm not sure if the test is right; if it is then this check makes sense. If not, we should change the test @bezoerb

Copy link
Collaborator

Choose a reason for hiding this comment

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

The isRemote function shouldn't intentionally receive a vinyl object. Until we find out where the vinyl is coming from the test should be something like this:

if (isVinyl(href)) {
   // Check custom attribute set in the vinylize function
  return Boolean(href.remote)
}

@XhmikosR XhmikosR marked this pull request as ready for review December 1, 2019 12:38
Makes the intention clearer.
@XhmikosR
Copy link
Collaborator Author

XhmikosR commented Dec 5, 2019

@bezoerb please check now, you should see the failure

@XhmikosR XhmikosR closed this May 21, 2020
@XhmikosR XhmikosR deleted the master-xmr-starts-ends branch May 21, 2020 15:28
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

2 participants