search

iterative / PyDrive2

Google Drive API Python wrapper library. Maintained fork of PyDrive.
https://docs.iterative.ai/PyDrive2
Other
570 stars 70 forks source link

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

Closed shcheklein closed 1 year ago

shcheklein commented 1 year ago

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 commented 1 year ago

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.

efiop commented 1 year ago

Side note: please consider including the "subsystem" name that you are modifying in the PR title. https://github.com/iterative/PyDrive2/pull/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 commented 1 year ago

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

efiop commented 1 year ago

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 commented 1 year ago

@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).