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

Make sure the sources are correct when information is aggregated #787

Merged
merged 5 commits into from
Jun 27, 2023

Conversation

carmenbianca
Copy link
Member

@carmenbianca carmenbianca commented Jun 25, 2023

Since #778, the source information is incorrect. This was flagged before here.

Also fixes #753

@carmenbianca carmenbianca force-pushed the info-correct-source branch 2 times, most recently from ee0d294 to 3ccdec1 Compare June 25, 2023 13:06
@carmenbianca carmenbianca marked this pull request as ready for review June 25, 2023 13:08
@carmenbianca carmenbianca force-pushed the info-correct-source branch 4 times, most recently from 2524fd0 to 95c8385 Compare June 25, 2023 15:26
Comment on lines 152 to 154
def contains_copyright_or_licensing(self) -> bool:
"""Either *spdx_expressions* or *copyright_lines* is non-empty."""
return bool(self.spdx_expressions or self.copyright_lines)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I think we should keep this method (+ its tests)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would also be in favor of leaving it in I think.

But otherwise, everything looks good to me. So feel free to merge when it suits you.

Copy link
Member

@linozen linozen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small thing I just noticed that should be included here perhaps is an explanation of how this kind of breaks JSON as in the JSON output there really is only one source given.

Or would you want to do that in a separate PR or with the next release?

@carmenbianca
Copy link
Member Author

One small thing I just noticed that should be included here perhaps is an explanation of how this kind of breaks JSON as in the JSON output there really is only one source given.

Au contraire :) Here's a part of the output when running reuse lint --json on this repository:

    {
      "path": "po/nl.po",
      "copyrights": [
        {
          "value": "2023 Free Software Foundation Europe e.V. <https://fsfe.org>",
          "source": ".reuse/dep5",
          "source_type": "dep5"
        },
        {
          "value": "SPDX-FileCopyrightText: 2018, 2020 Carmen Bianca Bakker",
          "source": "po/nl.po",
          "source_type": "file-header"
        },
        {
          "value": "SPDX-FileCopyrightText: 2017, 2020 Andr\u00e9 Ockers",
          "source": "po/nl.po",
          "source_type": "file-header"
        }
      ],
      "spdx_expressions": [
        {
          "value": "GPL-3.0-or-later",
          "source": ".reuse/dep5",
          "source_type": "dep5"
        },
        {
          "value": "GPL-3.0-or-later",
          "source": "po/nl.po",
          "source_type": "file-header"
        }
      ]
    },

This PR fixes exactly what you just described :D

@linozen
Copy link
Member

linozen commented Jun 27, 2023

This PR fixes exactly what you just described :D

Amazing! Then this is ready for a merge IMHO.

@carmenbianca
Copy link
Member Author

Let's first undo the removal of the method I flagged, and then let's merge :D

Also added contains_info, which may be more pertinent in the future.

Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
Otherwise on Windows, thse get backwards slashes.

Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
@carmenbianca carmenbianca merged commit 2b134b8 into fsfe:main Jun 27, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove potentially superfluous attributes in _File class
2 participants