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

Raise error when passed invalid file paths #394

Merged
merged 8 commits into from
Aug 10, 2023
Merged

Raise error when passed invalid file paths #394

merged 8 commits into from
Aug 10, 2023

Conversation

stufro
Copy link
Contributor

@stufro stufro commented Jul 26, 2023

#388

See comment on #388 for my thought process.

  • I had to change spec/ameba/cli/cmd_spec.cr to use real files to prevent the error being thrown there.
    • This also involved changing line 8 of that spec to use a config file which disables some rules defined in spec/spec_helper.cr which always adds errors.

@stufro stufro mentioned this pull request Jul 26, 2023
Open
veelenga
veelenga reviewed Jul 27, 2023
View reviewed changes
src/ameba/glob_utils.cr Outdated Show resolved Hide resolved
spec/ameba/cli/cmd_spec.cr Outdated Show resolved Hide resolved
veelenga
veelenga reviewed Aug 5, 2023
View reviewed changes
src/ameba/config.cr Outdated Show resolved Hide resolved
veelenga
veelenga reviewed Aug 5, 2023
View reviewed changes
Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

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

Just a few minor suggestions on my end. Rest LGTM

spec/ameba/config_spec.cr Outdated Show resolved Hide resolved
src/ameba/config.cr Outdated Show resolved Hide resolved
@stufro stufro requested a review from veelenga August 5, 2023 18:30
veelenga
veelenga approved these changes Aug 6, 2023
View reviewed changes
Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

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

LGTM

@veelenga veelenga requested a review from Sija August 6, 2023 15:43
@veelenga veelenga merged commit 18d193b into crystal-ameba:master Aug 10, 2023
2 of 4 checks passed
@stufro stufro deleted the 388-raise-on-invalid-file-path branch August 10, 2023 13:54
@Sija
Copy link
Member

Sija commented Dec 25, 2023 •
edited
Loading

This broke ameba's github action :( Also, I believe it's incorrect to raise for invalid paths in Globs or Excluded - these can be potentially empty or missing and neither of these should be a reason to raise an error and halt the execution of the whole program.

@Sija Sija added this to the 1.6.0 milestone Dec 26, 2023
@stufro
Copy link
Contributor Author

stufro commented Dec 27, 2023

Ah sorry about this @Sija. I'm happy to have a look at resolving this but won't likely have time for several weeks so it might be best to revert this PR for now.

Sija added a commit that referenced this pull request Dec 27, 2023
@Sija
Revert "Merge pull request #394 from stufro/388-raise-on-invalid-file…
0962cda 
…-path"

This reverts commit 18d193b, reversing
changes made to 7b8316f.
Sija added a commit that referenced this pull request Dec 28, 2023
@Sija
Revert "Merge pull request #394 from stufro/388-raise-on-invalid-file…
26d9bc0 
…-path"

This reverts commit 18d193b, reversing
changes made to 7b8316f.
@Sija
Copy link
Member

Sija commented Dec 28, 2023

@stufro No worries, let's revert this PR for now and take care of it l8r.

Sija added a commit that referenced this pull request Dec 28, 2023
@Sija
Merge pull request #435 from crystal-ameba/revert-pr-394
954345d 
Revert "Merge pull request #394 from stufro/388-raise-on-invalid-file…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veelenga veelenga veelenga approved these changes

@Sija Sija Awaiting requested review from Sija

Assignees
No one assigned
Labels
breaking change
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@stufro @Sija @veelenga