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

Set content picks default title/mime if file is created for the first time #276

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

shcheklein
Copy link
Member

Fixes #272 . Followup after #273 .

Trying a bit less dramatic change. It should not be breaking backward compatibility and is fixing #272 as a regression.

@shcheklein shcheklein added the bug Something isn't working label Apr 10, 2023
@shcheklein shcheklein self-assigned this Apr 10, 2023
@shcheklein shcheklein temporarily deployed to internal April 10, 2023 18:48 — with GitHub Actions Inactive
@shcheklein shcheklein temporarily deployed to internal April 10, 2023 21:43 — with GitHub Actions Inactive
@shcheklein shcheklein temporarily deployed to internal April 10, 2023 22:02 — with GitHub Actions Inactive
@shcheklein shcheklein requested a review from efiop April 10, 2023 22:25
@shcheklein
Copy link
Member Author

hey @efiop sorry for bothering you, decided that it can be done in a bit less aggressive mode and probably would just a regression fix vs a incompatible change.

docs/filemanagement.rst Outdated Show resolved Hide resolved
@efiop
Copy link
Contributor

efiop commented Apr 10, 2023

Side note: please consider including the "subsystem" name that you are modifying in the PR title. #273 should've been something like files: don't magically set filename on set file content. The current PR title is completely not descriptive. Also fix(123) stuff is kinda useless in PR title as it is not informative (issue mention in the PR description is informative). Not including a subsystem/component prefix is fine too.

I'm not sure what conventions you are following in pydrive2, but it surely doesn't follow https://dvc.org/doc/contributing/core#commit-message-format-guidelines Am I missing this fix(123) stuff from some other iterative guide that I'm not aware of?

@shcheklein
Copy link
Member Author

I'm not sure what conventions you are following in pydrive2

@efiop it loosely based on this https://www.conventionalcommits.org/en/v1.0.0-beta.2/ . But it's not required here. It's not set as a standard or a requirement.

I agree that it can be better - I'll update it

@shcheklein shcheklein changed the title fix(272): trying less disruptive change fix(files): set content picks default title/mime if file is created for the first time Apr 11, 2023
@shcheklein shcheklein changed the title fix(files): set content picks default title/mime if file is created for the first time Set content picks default title/mime if file is created for the first time Apr 11, 2023
@efiop efiop temporarily deployed to internal April 11, 2023 00:12 — with GitHub Actions Inactive
@efiop efiop merged commit 77e4b65 into main Apr 11, 2023
@efiop efiop deleted the fix-272-2 branch April 11, 2023 00:22
@efiop
Copy link
Contributor

efiop commented Apr 11, 2023

It's not set as a standard or a requirement.

Not setting it as a requirement for everyone. Just thought we at least were in sync between us, but looks like we are not 🙁 Noted and I won't pay as much attention to it next time 👍

@shcheklein
Copy link
Member Author

@efiop nw. It was based on the commit standard since I didn't pay enough attention and PR title got auto-generated from a commit message (that was bad by itself since I planned to amend it later- I was literally first trying things around and forgot to amend it). So, to clarify I don't think it was good, or that I like that standard for PR titles - it probably fits in some way, and if we use squash it becomes indeed somewhat useful to use that standard, but I'm totally fine with a regular DVC standard as well.

Standards aside - title was not descriptive anyways, and was not following the (any) standard anyways, so good catch. I think this is the most important part here. Details don't matter much (to me).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#223 silently renames file on update by ID
2 participants