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

Change sqlite DB path default to data directory #6198

Merged
merged 3 commits into from
Feb 27, 2019

Conversation

jolheiser
Copy link
Member

As titled, split from #6176

Signed-off-by: jolheiser <john.olheiser@gmail.com>
@techknowlogick techknowlogick added this to the 1.8.0 milestone Feb 27, 2019
@techknowlogick techknowlogick added type/refactoring Existing code has been cleaned up. There should be no new functionality. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! labels Feb 27, 2019
@techknowlogick techknowlogick changed the title Change DB path default to data directory Change sqlite DB path default to data directory Feb 27, 2019
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 27, 2019
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@b03d780). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6198   +/-   ##
=========================================
  Coverage          ?   38.86%           
=========================================
  Files             ?      354           
  Lines             ?    50192           
  Branches          ?        0           
=========================================
  Hits              ?    19508           
  Misses            ?    27859           
  Partials          ?     2825
Impacted Files Coverage Δ
models/models.go 51.96% <100%> (ø)

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 b03d780...9889a6c. Read the comment docs.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 27, 2019
@lunny
Copy link
Member

lunny commented Feb 27, 2019

And why not filepath.Join?

@jolheiser
Copy link
Member Author

I was unaware there were two different common join methods. Go has a few of these interesting "features" I've noticed.

After reading the docs and differences, filepath probably makes more sense here, though throughout the code I see both methods being used somewhat interchangeably.

Would you prefer I change it to filepath?

@jolheiser
Copy link
Member Author

@lunny @techknowlogick I doubt it would change your review, but just an FYI that I changed path.Join to filepath.Join 👍

@MCF
Copy link
Contributor

MCF commented Feb 27, 2019

FWIW, I did some research into path vs. filepath in go, and how it impacts paths in Windows. Turns out it is not that much of a problem in most cases:

#2845

There are a few additional notes in the pull request that fixed that issue if you are interested in all the gory details.

@jolheiser
Copy link
Member Author

Thanks for the info! That's more or less what I read when looking in to it as well.
Paths to a file (or other system location) should generally use filepath

As Go explains it:

The filepath package uses either forward slashes or backslashes, depending on the operating system. To process paths such as URLs that always use forward slashes regardless of the operating system, see the path package.

@techknowlogick techknowlogick merged commit 6cc11ec into go-gitea:master Feb 27, 2019
@techknowlogick
Copy link
Member

Thanks for making this change 😃

@jolheiser jolheiser deleted the db_path branch February 27, 2019 16:56
@MCF
Copy link
Contributor

MCF commented Feb 27, 2019

It is true that filepath is preferable. But I found in my testing that Go is very forgiving with the slashes, so you can mix and match slashes as you wish and Go will "do the right thing" in most cases on any operating systems. In other words you can actually be pretty lazy about using path instead of filepath and get away with it in almost all cases.

The fly in the ointment that I found was UNC paths (i.e. Windows files share paths like: \\my-cool-share\data\test.txt). Path does not always "do the right thing" with those paths. But filepath does.

But fundamentally everyone is right in saying that filepath should be used for paths to the filesystem. I was just trying to provide context around why it still (mostly) works if you don't.

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants