Skip to content

feat: Add filename normalizing#214

Merged
jakebolam merged 8 commits into
bundlewatch:masterfrom
beeequeue:filename-sanitizing
Sep 9, 2020
Merged

feat: Add filename normalizing#214
jakebolam merged 8 commits into
bundlewatch:masterfrom
beeequeue:filename-sanitizing

Conversation

@beeequeue

Copy link
Copy Markdown

What kind of change does this PR introduce?

Feature

Did you add tests for your changes?

I added some tests for the normalizing, but couldn't find any way to test it further than I did. Please let me know if anything else should be added.

If relevant, link to documentation update:

TODO

Summary

Some bundles may have hashes in their filenames - this breaks comparisons since files appear to be deleted instead of modified when the hashes change.

This adds an option for normalizing these filenames to fix this iseue.

I also thought about adding an enum option, so you can easily remove hashes without having to write a new Regex for it.

e.g.

$ yarn bundlewatch --normalize hash

Does this PR introduce a breaking change?

No

Other information

While adding the config, I made the validators able to alter the configuration to be valid to easily parse string regexes from JSON files or CLI options.

@beeequeue beeequeue changed the title Filename sanitizing feat: Add filename normalizing Aug 12, 2020
@beeequeue

Copy link
Copy Markdown
Author

Just saw #192, oops.

@beeequeue beeequeue closed this Aug 12, 2020
@cheapsteak

cheapsteak commented Aug 16, 2020

Copy link
Copy Markdown
Contributor

@beeequeue yours looks like a fine solution, you also don't have merge conflicts on the branch I think?
might as well reopen this/leave it up and see which one @jakebolam might prefer XD

@beeequeue beeequeue reopened this Aug 16, 2020
@jakebolam

Copy link
Copy Markdown
Member

Thank you for doing this, this is an awesome feature.

@jakebolam

Copy link
Copy Markdown
Member

I'll get this merged, any chance you want to add some docs for this?
https://github.com/bundlewatch/bundlewatch.io

@jakebolam

Copy link
Copy Markdown
Member

Fixes: #30

@jakebolam jakebolam merged commit 6b53e82 into bundlewatch:master Sep 9, 2020
@cheapsteak

cheapsteak commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

🎉
@beeequeue do you happen to have an example of a cli command including the regex for removing the hash?

@cheapsteak

Copy link
Copy Markdown
Contributor

ooo nvm this worked for us (added to the config in package.json)
"normalizeFilenames": "^.+?(\\.\\w+)\\.(?:js|css)$"

sweet!!!

@beeequeue

Copy link
Copy Markdown
Author

Yes I will open a PR in the website to add documentation

@beeequeue beeequeue deleted the filename-sanitizing branch September 9, 2020 15:56
@beeequeue

Copy link
Copy Markdown
Author

@cheapsteak Could you confirm for me that this working as intended for you? I'm seeing some weird behaviour where it still says the files are deleted and re-added even though they have the same name...

Example

@cheapsteak

cheapsteak commented Sep 16, 2020

Copy link
Copy Markdown
Contributor

@beeequeue I ran into the same issue 😞 (see #30 (comment) )

@cheapsteak

cheapsteak commented Sep 16, 2020

Copy link
Copy Markdown
Contributor

my guess is that the problem might be here:

const baseBranchFileDetails = await bundlewatchService.getFileDetailsForBaseBranch()
await bundlewatchService.saveFileDetailsForCurrentBranch({
fileDetailsByPath: currentBranchFileDetails,
trackBranches: ci.trackBranches,
})
const results = analyze({
currentBranchFileDetails,
baseBranchFileDetails,
baseBranchName: ci.repoBranchBase,
normalizeFilenames,
})

The file path normalization is being done inside the analyze call, but file details are sent into saveFileDetailsForCurrentBranch before normalization/analyze runs

I think we might need to move normalization into getLocalFileDetails instead

@cheapsteak

Copy link
Copy Markdown
Contributor

@jakebolam @beeequeue #238 should fix this (no api changes)

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.

3 participants