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.

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

feat: file not found error #802

Merged
merged 8 commits into from
Apr 1, 2020
Merged

feat: file not found error #802

merged 8 commits into from
Apr 1, 2020

Conversation

HugoSentelien
Copy link
Contributor

@HugoSentelien HugoSentelien commented Nov 21, 2019 •
edited by P0lip
Loading

Display when the file specification file is not found.

Changes
Changed return type of listFiles.
Called fg for each files in listFiles
Created error in linter.ts for each file not found.

Fixes #801.

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Screenshots

image

Test not passing

I know this PR will not be accepted as it is, because this test isn't going though because of my change:
image
I get it is because I am calling fg for each file while the previous one was for every files at once.
But, I have no idea how to do it to keep a test with the same usefulness.

I did change the same file to update the test "returns file paths" because of the change of return type of listFiles.

Thank you in advance for reviewing this :)

@P0lip
Copy link
Contributor

P0lip commented Dec 16, 2019

My sincere apologies for the lack of any review of this one.
I'll take a look at it this week. Looks like a nice improvement. Thank you.

@P0lip P0lip assigned P0lip and unassigned P0lip Dec 16, 2019
@P0lip P0lip self-requested a review December 16, 2019 14:54
P0lip
P0lip reviewed Dec 26, 2019
View reviewed changes
Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

Nice work! Thanks and my apologies for a (very) delayed code review.
A couple of comments, but looking well otherwise.

src/cli/services/linter/utils/listFiles.ts Outdated Show resolved Hide resolved
src/cli/services/linter/linter.ts Outdated Show resolved Hide resolved
src/cli/services/linter/linter.ts Outdated Show resolved Hide resolved
src/cli/services/linter/linter.ts Outdated Show resolved Hide resolved
src/cli/services/linter/linter.ts Outdated Show resolved Hide resolved
src/cli/services/linter/linter.ts Outdated Show resolved Hide resolved
src/cli/services/linter/linter.ts Outdated Show resolved Hide resolved
@HugoSentelien
Copy link
Contributor Author

Thank you for the review !
The syntax for the Promise.all will really help to make it faster instead of making each search waiting for the previous one 😃

I will be sure to change & commit this weekend (I don't have the time to test right everything right now) 😉

@P0lip
Copy link
Contributor

P0lip commented Jan 15, 2020

@HugoSentelien could you allow external contributions to your branch? :)
I'd finish this one for you if you don't mind :)

@P0lip P0lip added the enhancement New feature or request label Jan 15, 2020
@P0lip P0lip changed the title File not found error feat: file not found error Jan 15, 2020
@HugoSentelien
Copy link
Contributor Author

@P0lip Thank you so much for the suggestion, sorry for not making it yet, it seems like something is coming up every day since some weeks (the worst was when I had to see for adding custom type and their rule in spectral, while knowing I had to update this branch without having the time xD).

Could you guide me on how to give you this authorization please? Because it seems like the authorization is already granted from my UI 👀
access_git

@P0lip
Copy link
Contributor

P0lip commented Feb 24, 2020

Could you guide me on how to give you this authorization please? Because it seems like the authorization is already granted from my UI eyes

Oops, sorry for lack of any response. Didn't notice you responded.
Hm, perhaps I need to set your remote.
I'll give it a try. Stay tuned.

@P0lip
Copy link
Contributor

P0lip commented Mar 29, 2020

I finally managed to wrap it up!
Decided not to error in the end. That being said, we still print out unmatched patterns.
Does it sound good to you? I went through your initial issue, and I am pretty sure it should meet your expectations, but double checking.
Moreover, I added a CLI flag, as I suggested before.
Let me know what you think. If it's all good, I'm going to merge it.
Thanks again for your work!

nulltoken
nulltoken reviewed Mar 29, 2020
View reviewed changes
src/cli/commands/lint.ts Outdated
'ignore-unmatched-globs': {
description: 'do not warn about unmatched glob patterns',
type: 'boolean',
default: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

@P0lip In the spirit of #678 could we make this default to false?

The idea being to provide the user with meaningful messages by default.

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, but this may result in some perf hit, as we might need to visit several directories multiple times.
Might be a not big deal, though. I'll consider doing it before we merge the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we could rename the option to show-unmatched-globs. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nulltoken bump

Copy link
Contributor

Choose a reason for hiding this comment

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

@P0lip Sorry. I overlooked this one :(

HugoSentelien and others added 5 commits April 1, 2020 15:18
@HugoSentelien @P0lip
File not found error
1d9a79d 
Change return type of listFiles.
Call fg for each files.
Create error in linter.ts for each file not found.
@HugoSentelien @P0lip
Update of test
db34de5 
Change of return value of lsitFiles = change of unit test
@P0lip
refactor: a few minor changes
cc46b42
@P0lip
chore: wip
817dc42
@P0lip
refactor: wrap it up
78a5f5f
@P0lip
Copy link
Contributor

P0lip commented Apr 1, 2020

Alright folks, since 5.3 is just around the corner, I'm proceeding with this one.
We can revisit it later if you have any feedback.
I did rename the property to make it aligned with the rest of flags.

@P0lip P0lip merged commit 6d9352b into stoplightio:develop Apr 1, 2020
P0lip added a commit that referenced this pull request Apr 1, 2020
@P0lip @HugoSentelien
feat: --show-unmatched-globs CLI flag (#802)
ea86170 
* File not found error

Change return type of listFiles.
Call fg for each files.
Create error in linter.ts for each file not found.

* Update of test

Change of return value of lsitFiles = change of unit test

* refactor: a few minor changes

* chore: wip

* refactor: wrap it up

* chore: rename to show-unmatched-globs

Co-authored-by: HugoSentelien <[email protected]>
@HugoSentelien
Copy link
Contributor Author

Thank you @P0lip and @nulltoken !

Thank you for your hardwork, and sorry for not having be there at all 🙄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nulltoken nulltoken nulltoken left review comments

@P0lip P0lip P0lip left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

File not found (CLI and Docker)
3 participants
@HugoSentelien @P0lip @nulltoken