Skip to content

Add the addFileNameHash option that allows users to remove the hash f… #4915

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
Merged
merged 4 commits into from
Jul 27, 2018
Merged

Add the addFileNameHash option that allows users to remove the hash f… #4915

merged 4 commits into from
Jul 27, 2018

Conversation

GabrielLomba
Copy link
Contributor

Add the addFileNameHash option that allows the removal of the hash from file names, as discussed in #4899.

@GabrielLomba GabrielLomba mentioned this pull request Jul 27, 2018
Closed
@codecov
Copy link

codecov bot commented Jul 27, 2018 •
edited
Loading

Codecov Report

Merging #4915 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4915      +/-   ##
=========================================
+ Coverage   92.89%   92.9%   +0.01%     
=========================================
  Files         119     119              
  Lines        8836    8837       +1     
=========================================
+ Hits         8208    8210       +2     
+ Misses        628     627       -1
Impacted Files Coverage Δ
src/Options/Definitions.js 100% <ø> (ø) ⬆️
src/Controllers/index.js 96.55% <100%> (ø) ⬆️
src/Controllers/FilesController.js 94.87% <100%> (+0.13%) ⬆️
src/RestWrite.js 93.14% <0%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0363728...a08ac69. Read the comment docs.

flovilmart
flovilmart suggested changes Jul 27, 2018
View reviewed changes
Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

Can we make it the opposite please? And call it ‘preserveFileName’

@georgesjamous
Copy link
Contributor

georgesjamous commented Jul 27, 2018 •
edited
Loading

@flovilmart just a thought.
Since in parse-server we mostly rely on files rather than names of the files, how about we extend this to use both when its required by the business.
‘preserveFileNamesIfPossible’ and test if the file name is empty "" we would generate, if not we would leave it the same ?
Otherwise if 'preserveFileNamesIfPossible' is false (default), we always generate. (like it is now)

@flovilmart
Copy link
Contributor

I’m not for adding extra logic that can royally introduce unexpected behaviors for an unknown and non required use case. YAGNI.

@georgesjamous
Copy link
Contributor

Fair enough 👍

@GabrielLomba
Copy link
Contributor Author

GabrielLomba commented Jul 27, 2018 •
edited
Loading

@flovilmart The requested changes were made.

@flovilmart
Copy link
Contributor

@GabrielLomba thanks for the changes, it's looking good. Can you revert the changes in the package-lock.json file please? I'M not sure they are relevant to this PR :0

@GabrielLomba
Copy link
Contributor Author

@flovilmart Done.

flovilmart
flovilmart approved these changes Jul 27, 2018
View reviewed changes
Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@flovilmart flovilmart merged commit 6a151ee into parse-community:master Jul 27, 2018
@flovilmart
Copy link
Contributor

Perfect thansk!

JacobJT
JacobJT reviewed Jul 27, 2018
View reviewed changes
spec/FilesController.spec.js
done();
});

it("should not add a unique hash to the file name when the preserveFileName option is false", (done) => {
Copy link

Choose a reason for hiding this comment

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

false should be true in the description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. My mistake. Should I make another PR fixing it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep go ahead ;)

flovilmart pushed a commit that referenced this pull request Aug 12, 2018
@GabrielLomba @flovilmart
Add the addFileNameHash option that allows users to remove the hash f… (
cc4b5fa 
#4915)

* Add the addFileNameHash option that allows users to remove the hash from file names

* Change option name to preserveFileName

* Revert changes to package-lock.json
flovilmart pushed a commit that referenced this pull request Aug 12, 2018
@GabrielLomba @flovilmart
Add the addFileNameHash option that allows users to remove the hash f… (
37e6459 
#4915)

* Add the addFileNameHash option that allows users to remove the hash from file names

* Change option name to preserveFileName

* Revert changes to package-lock.json
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
@GabrielLomba @flovilmart
Add the addFileNameHash option that allows users to remove the hash f… (
698eb4a 
parse-community#4915)

* Add the addFileNameHash option that allows users to remove the hash from file names

* Change option name to preserveFileName

* Revert changes to package-lock.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flovilmart flovilmart flovilmart approved these changes

@JacobJT JacobJT JacobJT left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GabrielLomba @georgesjamous @flovilmart @JacobJT