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

Mark pdm.lock as generated #1611

Closed
BlueGlassBlock opened this issue Jan 12, 2023 · 11 comments · Fixed by #1612
Closed

Mark pdm.lock as generated #1611

BlueGlassBlock opened this issue Jan 12, 2023 · 11 comments · Fixed by #1612
Labels
⭐ enhancement Improvements for existing features ❓ help wanted Extra attention is needed

Comments

@BlueGlassBlock
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Marking pdm.lock with @generated helps various tools to identify and skip it (see https://generated.at)

Although we can specify it in .gitattributes, it would be convenient to mark it directly in the file.

Describe the solution you'd like

Add a header to pdm.lock including @gererated in the first line.

For reference, here are the headers from Poetry and Cargo:

python-poetry/poetry#2034

# This file is automatically @generated by Poetry and should not be changed by hand.

rust-lang/cargo#6180

# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
@BlueGlassBlock BlueGlassBlock added the ⭐ enhancement Improvements for existing features label Jan 12, 2023
@frostming
Copy link
Collaborator

PR welcome

@frostming frostming added the ❓ help wanted Extra attention is needed label Jan 12, 2023
@BlueGlassBlock
Copy link
Contributor Author

BlueGlassBlock commented Jan 12, 2023

diff --git a/src/pdm/project/lockfile.py b/src/pdm/project/lockfile.py
index 20806f9..dc7ea44 100644
--- a/src/pdm/project/lockfile.py
+++ b/src/pdm/project/lockfile.py
@@ -1,4 +1,10 @@
 from pdm.project.toml_file import TOMLBase
+from tomlkit import comment
+
+GENERATED_COMMENTS = [
+    "This file is @generated by PDM.",
+    "It is not intended for manual editing.",
+]


 class Lockfile(TOMLBase):
@@ -13,6 +19,8 @@ class Lockfile(TOMLBase):
         return self._data.get("metadata", {}).get("lock_version", "")

     def write(self, show_message: bool = True) -> None:
+        for index, line in enumerate(GENERATED_COMMENTS):
+            self._data.body.insert(index, (None, comment(line)))
         super().write()
         if show_message:
             self.ui.echo(f"Changes are written to [success]{self._path.name}[/].")

Would it be too hacky?

@frostming
Copy link
Collaborator

frostming commented Jan 12, 2023

@BlueGlassBlock You can use self._data._insert_at(index, comment(line)), and remember to handle the situation if the comments already exist

@BlueGlassBlock
Copy link
Contributor Author

BlueGlassBlock commented Jan 12, 2023

@BlueGlassBlock You can use self._data._insert_at(index, comment(line)), and remember to handle the situation if the comments already exist

TOMLDocument._insert_at requires an extra key argument which can't be None, TOMLDocument.append (which TOMLDocument.add will call) will only update its body when supplying a comment, if I'm interpreting correctly.

@frostming
Copy link
Collaborator

frostming commented Jan 12, 2023

@BlueGlassBlock you're right. Be aware that directly adding to body will mess up the indexes stored in Container, which also needs to be updated, or alternatively, reread the document.

@BlueGlassBlock
Copy link
Contributor Author

diff --git a/src/pdm/project/lockfile.py b/src/pdm/project/lockfile.py
index 20806f9..660b631 100644
--- a/src/pdm/project/lockfile.py
+++ b/src/pdm/project/lockfile.py
@@ -1,4 +1,11 @@
 from pdm.project.toml_file import TOMLBase
+import tomlkit
+from typing import Mapping, Any
+
+GENERATED_COMMENTS = [
+    "This file is @generated by PDM.",
+    "It is not intended for manual editing.",
+]


 class Lockfile(TOMLBase):
@@ -12,6 +19,12 @@ class Lockfile(TOMLBase):
     def file_version(self) -> str:
         return self._data.get("metadata", {}).get("lock_version", "")

+    def set_data(self, data: Mapping[str, Any]) -> None:
+        self._data = tomlkit.document()
+        for line in GENERATED_COMMENTS:
+            self._data.append(None, tomlkit.comment(line))
+        self._data.update(data)
+
     def write(self, show_message: bool = True) -> None:
         super().write()
         if show_message:

This one seemed to be the cleanest (I ran pdm lock and pdm update several times to check its effect). I'm checking that self._data.update(data) won't cause trouble.

@frostming
Copy link
Collaborator

@BlueGlassBlock that is way cleaner, LGTM

@ProgramRipper
Copy link
Contributor

Maybe modify pdm.cli.utils.format_lockfile() would be better? As all lock toml documents are created by it.

diff --git a/src/pdm/cli/utils.py b/src/pdm/cli/utils.py
index 9a3182be..bb11c076 100644
--- a/src/pdm/cli/utils.py
+++ b/src/pdm/cli/utils.py
@@ -495,6 +495,8 @@ def format_lockfile(
             if array:
                 file_hashes.add(key, array)
     doc = tomlkit.document()
+    doc.add(tomlkit.comment("This file is @generated by PDM."))
+    doc.add(tomlkit.comment("It is not intended for manual editing."))
     doc.add("package", packages)  # type: ignore
     metadata = tomlkit.table()
     metadata.add("files", file_hashes)

But for it to work, this change must be made, because TOMLBase.set_data original behaviour will remove all standalone comment:

diff --git a/src/pdm/project/toml_file.py b/src/pdm/project/toml_file.py
index d157b07d..f650e624 100644
--- a/src/pdm/project/toml_file.py
+++ b/src/pdm/project/toml_file.py
@@ -24,8 +24,11 @@ class TOMLBase(TOMLFile):
 
     def set_data(self, data: Mapping[str, Any]) -> None:
         """Set the data of the TOML file."""
-        self._data = tomlkit.document()
-        self._data.update(data)
+        if isinstance(data, tomlkit.TOMLDocument):
+            self._data = data
+        else:
+            self._data = tomlkit.document()
+            self._data.update(data)
 
     def reload(self) -> None:
         self._data = self.read()

@ProgramRipper
Copy link
Contributor

BTW, is this a expected behaviour?

because TOMLBase.set_data original behaviour will remove all standalone comment:

@frostming
Copy link
Collaborator

BTW, is this a expected behaviour?

because TOMLBase.set_data original behaviour will remove all standalone comment:

Yes, set_data means to overwrite, all existing contents should be removed.

@BlueGlassBlock
Copy link
Contributor Author

remember to handle the situation if the comments already exist

I found that do_add calls project.write_lockfile twice in total (once by do_lock and once by calling directly), however it's not a problem if set_data removes free-flying comments (as current behavior).

Fortunately, tomlkit didn't tweak the Container.update behaviour...

frostming pushed a commit that referenced this issue Jan 12, 2023
* feat(lockfile): add `@generated` comment

Resolve #1611.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐ enhancement Improvements for existing features ❓ help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants