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.

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

Added Static File Handler #674

Merged

Conversation

KedarisettiSreeVamsi
Copy link
Contributor

@KedarisettiSreeVamsi KedarisettiSreeVamsi commented Jun 2, 2024

Added Static File Handling
If a public directory is found then it will create a public endpoint by default
If we want to added any other folders can use AddStaticFiles endpoint and then if endpoint and directory is provided then it will check whether the particular endpoint is available otherwise throws an error

Closes: #502.

pkg/gofr/gofr.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

Thanks for doing the changes I suggested

@KedarisettiSreeVamsi
Copy link
Contributor Author

Let me know if any changes can be done to accommodate more functionality.

pkg/gofr/gofr.go Outdated Show resolved Hide resolved
pkg/gofr/gofr.go Outdated Show resolved Hide resolved
pkg/gofr/gofr.go Outdated Show resolved Hide resolved
@Umang01-hash
Copy link
Contributor

@KedarisettiSreeVamsi Thankyou for your contribution.
Please add consider adding tests for any new code added as not doing it will decrease the overall code coverage.
Also, in my opinion we don't actually need an example of how to serve static files. Your example is small and simple so a better way is to add a documentation for this.

You can add it in the advanced-guide directory under the docs directory of the repository.

docs/advanced-guide/serving-static-files/page.md Outdated Show resolved Hide resolved
docs/advanced-guide/serving-static-files/page.md Outdated Show resolved Hide resolved
pkg/gofr/http/router.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

Small typo

docs/advanced-guide/serving-static-files/page.md Outdated Show resolved Hide resolved
@vipul-rawat
Copy link
Member

vipul-rawat commented Jun 5, 2024

@KedarisettiSreeVamsi please add unit tests for the feature you've implemented!
Please fix the linters also!

@KedarisettiSreeVamsi
Copy link
Contributor Author

Hi @vipul-rawat,
Can you please mention how I can write unit tests without a static folder in the same folder as test file? Should I use a file within the folder for the testing purpose?

@KedarisettiSreeVamsi
Copy link
Contributor Author

I am pretty new to this so was the reason I am asking this question.

@vipul-rawat
Copy link
Member

@KedarisettiSreeVamsi I think we can create a tmp folder and when cleaning up we can remove all the tmp files created!

pkg/gofr/gofr_test.go Outdated Show resolved Hide resolved
pkg/gofr/gofr_test.go Outdated Show resolved Hide resolved
pkg/gofr/gofr_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

LGTM 👍

.gitignore Outdated Show resolved Hide resolved
docs/advanced-guide/serving-static-files/page.md Outdated Show resolved Hide resolved
docs/advanced-guide/serving-static-files/page.md Outdated Show resolved Hide resolved
pkg/gofr/gofr_test.go Show resolved Hide resolved
pkg/gofr/gofr_test.go Outdated Show resolved Hide resolved
@KedarisettiSreeVamsi
Copy link
Contributor Author

Can someone resolve the conflict of docs/reference/configs/page.md as it is blocking all the tests? I tried to fix it but unfortunately it Is not getting fixed.

Copy link
Member

@aryanmehrotra aryanmehrotra left a comment

Choose a reason for hiding this comment

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

  1. Remove helper and constant files. (fixed)
  2. Lot of lines are not covered (coverage is not 100% for the newly added code)
  3. ExcludeFiles need to be unexported (we can move the code to add openapi.json in the router itself) (fixed)
  4. FileDirectory needs to be renamed, doesn't seem right
  5. ExcludeExtensions, HideDotFiles can be removed for now, I dont find a valid usecase. (IMO user can just not keep that file if they dont want it to be rendered) (fixed)
  6. DirectoryListing can be renamed to ListDirectories or ListDirectoryFiles
  7. A lot of functions/methods are being exported which can be unexported (fixed)
  8. .env Config STATIC_DIRECTORY_LISTING can be removed (fixed)

@ccoVeille
Copy link
Contributor

  1. ExcludeExtensions, HideDotFiles can be removed for now, I dont find a valid usecase. (IMO user can just not keep that file if they dont want it to be rendered) (fixed)

Seem safer and avoid potential breaking changes 👍

pkg/gofr/gofr.go Outdated Show resolved Hide resolved
pkg/gofr/gofr_test.go Outdated Show resolved Hide resolved
aryanmehrotra
aryanmehrotra previously approved these changes Jun 26, 2024
pkg/gofr/gofr_test.go Show resolved Hide resolved
pkg/gofr/gofr_test.go Outdated Show resolved Hide resolved
pkg/gofr/gofr_test.go Show resolved Hide resolved
@vipul-rawat vipul-rawat merged commit cd5b92e into gofr-dev:development Jun 26, 2024
9 checks passed
@srijan-27 srijan-27 mentioned this pull request Jun 28, 2024
This was referenced Jul 8, 2024
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.

Support for serving static files
6 participants