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

Don’t delete comments from the tops of INI-style files #3509

Closed
Jayman2000 opened this issue Nov 6, 2021 · 4 comments
Closed

Don’t delete comments from the tops of INI-style files #3509

Jayman2000 opened this issue Nov 6, 2021 · 4 comments

Comments

@Jayman2000
Copy link
Contributor

Jayman2000 commented Nov 6, 2021

Describe the project you are working on

A game that will eventually be FOSS. At the moment, the project folder mostly contains stuff made by me, but it also contains something from the asset library. In the future, I plan to incorporate assets from opengameart.org and (hopefully) pull requests from contributors.

Describe the problem or limitation you are having in your project

Copyright can get complicated fast. I want to clearly communicate the copyright status of files in my project so that people can reuse them easily. For each file in my project, I want to make the following clear:

  1. Who owns the copyright
  2. What license the file is under

For GDScript files, I can do this by using license headers or SPDX License-Identifier and FileCopyrightText tags. The problem is that I can’t include them INI-style files (project.godot, .tres files and .tscn files). INI-style files are allowed to have comments, but they get lost when Godot reads them.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Make it so that comments at the top of INI-style files are preserved. That way I could put license headers in INI-style files without Godot removing them. This would be kind of inconsistent (since comments in other parts of the file would still disappear), so it might make sense to give an error if comments appear anywhere else.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

  1. Godot needs to save a file to res://example.tscn.
  2. If that file already exists:
    A. Godot starts reading the file.
    B. Godot keeps reading until it finds the first byte that isn’t whitespace or part of a comment. Call this byte “B”.
    C. Godot truncates the file to B-1 bytes.
  3. Godot appends the data it was going to write to res://example.tscn.

Step 2B would allow for trailing whitespace, but it would also prevent people from losing comments because they included an extra space before the ; or included a blank line between comments.

If this enhancement will not be used often, can it be worked around with a few lines of script?

I’m not sure if this enhancement would be used often. I’ll definitely use it often, but I’m not sure if others would. That being said, I can think of a couple of workarounds, but none of them are a few lines of a script.

REUSE

.license files

REUSE allows you to include copyright and licensing information in .license files. For every INI-style file in my project, I could include a .license file. There’s a few problems with this idea:

  1. It would be a violation of the REUSE Spec. The Spec says “To implement this method, each plain text file that can contain comments MUST contain comments at the top of the file (comment header) that declare that file’s Copyright and Licensing Information.”
  2. It would result in a lot of extra .license files in my project folder.
  3. They could easily get lost if files are copied from my project to somewhere else.
  4. I’m not a lawyer, but I’m skeptical about whether or not a .license file is sufficient for applying a license. An SPDX-License-Identifier in an adjacent file is a far cry from your typical license header. In fact, the SPDX Spec recommends that you still include standard license headers in source files when using SPDX-License-Identifiers.
DEP5 files

REUSE also allows you to include copyright and licensing information in a DEP5 file. This would have all of the same problems that I listed above, except for number 2.

Include a file that says the copyright holder and license for each INI-style file

This seems like a lot of work, and it seems error prone. Also, there’s a reasonable chance that if one of my INI-style files gets copied, then that copying information won’t come with it.

An add-on

Definitely not a few lines of code. See below.

Is there a reason why this should be core and not an add-on in the asset library?

I actually did try to make an add-on that did this. It would look at its own config file and then based on the rules in that config file, it would add comments to files every time they were saved. There’s a couple of problems with the solution:

  1. Sometimes, Godot strips comments from INI-style files that you open, sometimes it doesn’t this meant that I had to try (and fail) to write code that would remove the comments for me.
  2. If someone copies a file into their project and they don’t have my add-on and an appropriate config file, then Godot will automatically get rid of the comments that my add-on added.
@Jayman2000 Jayman2000 changed the title Don’t delete comments at the tops of INI-style files Don’t delete comments from the tops of INI-style files Nov 6, 2021
@dalexeev
Copy link
Member

dalexeev commented Nov 7, 2021

I wanted to propose something similar. We can add the methods:

ConfigFile.set_top_comment(comment: String) -> void
ConfigFile.get_top_comment() -> String

But should we do the same with sections and keys?

@Jayman2000
Copy link
Contributor Author

I wanted to propose something similar. We can add the methods:

ConfigFile.set_top_comment(comment: String) -> void
ConfigFile.get_top_comment() -> String

I don’t think that those two methods would get through the proposal process. Here are the considerations that I think you’ll run into problems with:

2. How much support is the proposal receiving?

Evaluate the amount of support the proposal is receiving. This is an ongoing
analysis. If a proposal receives little support at first, it may receive
additional support later on.

I can only speak for myself, but I don’t like this idea. A comment is supposed to be something that the computer normally ignores. Those two methods make it easy to store data in the top comment and then access it later. Essentially, the top comment would look like a comment, but not be treated like a comment.

3. Can this proposal be implemented with an addon?

Evaluate whether it is possible for the proposal to be implemented in an addon.
If it is possible for the proposal to be in an addon, it is less likely to be
accepted.

Yes.

6. Does this proposal help users overcome a limitation?

Proposals that overcome a specific limitation are more likely to be accepted
than proposals that are just helpful. In short, need-to-have features will be
prioritized over nice-to-have features. Further, the core developers prioritize
changes that enable users to implement features themselves over implementing
those same features in core.

Not really. ConfigFiles are already good at storing Strings.

8. Can the feature be worked around in script with a few lines?

If the feature is only intended to save users a few lines of code it is unlikely
to be accepted.

Yes. You can just create a key that acts like a comment:

var cfg = ConfigFile.new()
cfg.set_value("#comment", "#comment", "Comment for entire ConfigFile")

cfg.set_value("Player1", "health", 100)
cfg.set_value("Player1", "lives", 5)
cfg.set_value("Player1", "level", 2)
cfg.set_value("Player1", "score", 2)

cfg.save("user://output.cfg")

user://output.cfg:

[#comment]

#comment="Comment for entire ConfigFile"

[Player1]

health=100
lives=5
level=2
score=2

But should we do the same with sections and keys?

I would just use “comment keys” for those:

cfg.set_value("#comment", "#comment", "Comment for entire ConfigFile")

cfg.set_value("Player1", "#section_comment", "Comment for the Player1 section")
cfg.set_value("Player1", "health", 100)
cfg.set_value("Player1", "lives", 5)
cfg.set_value("Player1", "#level_comment", "Comment for the level key")
cfg.set_value("Player1", "level", 2)
cfg.set_value("Player1", "#score_comment", "Comment for the score key")
cfg.set_value("Player1", "score", 2)

cfg.save("user://output.cfg")

user://output.cfg:

[#comment]

#comment="Comment for entire ConfigFile"

[Player1]

#section_comment="Comment for the Player1 section"
health=100
lives=5
#level_comment="Comment for the level key"
level=2
#score_comment="Comment for the score key"
score=2

@Calinou
Copy link
Member

Calinou commented Jul 21, 2022

Following the discussion about comments in #941, I suppose an optional parameter could be added to ConfigFile.set_value() so you can specify a comment to write before the property (on its own line). That, and set_top_comment() to specify a comment to write at the top of the ConfigFile.

Reading the comments back using ConfigFile shouldn't be supported from Godot, as comments should not be used to store information that affects how the ConfigFile works no matter what. (That said, nothing prevents you from doing so manually with the File class.)

@Jayman2000
Copy link
Contributor Author

Now that this issue with the REUSE Spec has been fixed, I’m not really interested in this proposal anymore. I’ll probably just use .license files and mention that the repo is REUSE compliant in its README.

Jayman2000 added a commit to Jayman2000/jasons-pre-commit-hooks that referenced this issue Jan 10, 2024
I’ve decided to completely revamp how I do copying information. Going
forward, this repo is going to be an exemplar. I’m going to (eventually)
make all of my repos declare their copying information similarly to how
this one does. My current system for declaring copying information is a
big and inconvenient compromise. This new system for declaring copying
information is less of a compromise

In the past, I had considered complying with the REUSE Spec [1] for all
of my repos. I had decided against doing that for several reasons:

1. REUSE requires that I call 🅭🄍 a license. 🅭🄍 is not a license, it’s a
public domain dedication [2]. Confusingly, 🅭🄍 contains a public fallback
license [3], and I’ve seen at least one person call 🅭🄍 a license to
indicate that their work is available under that fallback license [4]. I
don’t want to call 🅭🄍 a license because I want to make sure that the
work is dedicated to the public domain as much as it possibly can be.

2. The REUSE Tool made it difficult to include proper attribution for
🅭🄍, and the developers of the REUSE Tool didn’t seem interested in
making it easier [5].

3. The REUSE Spec required that all Covered Files that could contain
comments did contain comments [6]. Godot allows you to put comments in
`.tscn` files, but it will delete them when you save the scene from the
editor [7]. This made it really inconvenient to follow the REUSE Spec
while working on a Godot project [8].

Those reasons are why I ended up using my current system. Unfortunately,
my current system is kind of annoying. For one thing, there’s nothing
quite like the REUSE Tool for my current system. I created
assert_contains_regex [9] as a stopgap solution. Once I had a version of
assert_contains_regex that worked well on Windows, I was going to create
a tool for my system that was more comparable to the REUSE Tool. At this
point, I still haven’t made assert_contains_regex work well on Windows,
and I still haven’t created the tool that would replace it.

A big problem with my current system is that it’s not standardized at
all. It’s specific to me and only me. Theoretically, someone else could
implement it for their projects, but that seems really unlikely. It
would be nice if I could use something standardized like REUSE. Then, I
wouldn’t have to create my own tools.

I’ve been thinking about the three problems that I had with REUSE, and
I’ve come up with solutions for all of them:

1. I can write some boilerplate text that makes my intentions clear. I
can say that 🅭🄍 is a public domain dedication. I clarify that when I say
“SPDX-License-Identifier: CC0-1.0”, I mean “dedicated to the public
domain using 🅭🄍.”

2. Now that this annoying loophole [10] has been fixed, I no longer need
to provide attribution for 🅭🄍 [11].

3. Now that this issue has been fixed [12], unstable versions of the
REUSE Spec no longer have that requirement. I’m still waiting for a
stable release, though.

Having solutions to those three problems makes using REUSE a legitimate
option, and using REUSE is going to be much less work than maintaining
tools for my current system.

---

On an unrelated note, this commit creates a file named “copying.md”.
Normally, I would create a file named “COPYING.md”, but the REUSE Tool
ignores files whose names start with “COPYING” or “LICENSE”. If this
feature request [13] gets implemented, then I’ll rename the file to
“COPYING.md”.

[1]: <https://reuse.software/spec/>
[2]: <https://creativecommons.org/faq/#how-do-cc-licenses-operate>
[3]: <https://creativecommons.org/publicdomain/zero/1.0/legalcode.en#fallback>
[4]: <https://comment.ctrl.blog/discussion/creative-commons-unicode-fallback-font>
[5]: <fsfe/reuse-tool#410>
[6]: <fsfe/reuse-docs#122>
[7]: <https://docs.godotengine.org/en/stable/contributing/development/file_formats/tscn.html#file-structure>
[8]: <godotengine/godot-proposals#3509>
[9]: <https://pagure.io/assert_contains_regex>
[10]: <https://jasonyundt.website/posts/cc0-loophole-annoying-for-2-years.html>
[11]: <https://github.com/creativecommons/cc-legal-tools-data/tree/7fe72a9f6c241f6ae8059751793e3471dd4fcebe#copying>
[12]: <fsfe/reuse-docs#122 (comment)>
[13]: <fsfe/reuse-tool#881>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants