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

Remove unnecessary condition in path stripping #1

Closed
JC3 opened this issue Apr 2, 2022 · 2 comments
Closed

Remove unnecessary condition in path stripping #1

JC3 opened this issue Apr 2, 2022 · 2 comments
Assignees
@JC3
Labels
enhancement New feature or request minor
Milestone
0.9.2

Comments

@JC3
Copy link
Owner

JC3 commented Apr 2, 2022 •
edited
Loading

Noticed in code review. Could be a bug; assess later to confirm:

harextract/harextract.html

Line 190 in b9a4dc0

ent.path = (ncommon < 0 ? '' : ent.pathcomps.slice(ncommon - 1).join('/'));

Possible that condition should be ncommon <= 0.
@JC3 JC3 added bug Something isn't working good first issue Good for newcomers labels Apr 2, 2022
@JC3 JC3 added this to the 0.9.2 milestone Apr 2, 2022
@JC3 JC3 changed the title Potential error when all files are in same path Potential error when all files are in different path Apr 3, 2022
@JC3
Copy link
Owner Author

JC3 commented Apr 3, 2022

Check is not necessary:

  • ncommon < 0 is definitely not possible.
  • ncommon <= 0 I think is not possible: unless a URL can be constructed where its pathname is empty (empty as in "", not "/"), then there will always be at least one common path component. I was not able to construct such a URL.

@JC3 JC3 changed the title Potential error when all files are in different path Remove unnecessary condition in path stripping Apr 3, 2022
@JC3 JC3 added enhancement New feature or request minor and removed bug Something isn't working labels Apr 3, 2022
JC3 added a commit that referenced this issue Apr 3, 2022
@JC3
remove unnecessary condition
9ac62f6 
#1
@JC3 JC3 closed this as completed Apr 3, 2022
JC3 added a commit that referenced this issue Apr 4, 2022
@JC3
[*] remove unnecessary code
665f243 
- fix #1 and #10
* squash into path strip simplification later
JC3 added a commit that referenced this issue Apr 4, 2022
@JC3
simplified path stripping logic
cc9e819 
- #8: simplified and cleaned up path stripping code
- #1: side effect removed unnecessary condition
- #10: side effect removed unnecessary code
@JC3
Copy link
Owner Author

JC3 commented Apr 4, 2022

Fixed by #8.

@JC3 JC3 removed the good first issue Good for newcomers label Apr 4, 2022
@JC3 JC3 self-assigned this Apr 4, 2022
JC3 added a commit that referenced this issue Apr 4, 2022
@JC3
0.9.2 feature set (#14)
18260f0 
0.9.2 feature set:

- [new] update check; quick and painless. displays a button up top if update available (#4)
- [fix] fixed tables not clearing on new file load (#12)
- [fix] displayed filename not in sync with open document (#11)
- [fix] reselecting the same file will now reload it (#13)
- [fix] corrected font size in zip compression level dropdown
- [improve] version and year bump
- [improve] update screenshot although... it's pretty much the same (#7)
- [code] simplified and cleaned up path stripping code (#1, #8, #10)
- [code] slightly more proper html
- [code] split up long jszip line (#5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JC3 JC3

Labels
enhancement New feature or request minor
Projects
None yet
Milestone
0.9.2
Development

No branches or pull requests
1 participant
@JC3