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

Remove unused local variables #641

Merged
merged 1 commit into from
Apr 19, 2022
Merged

Conversation

pzygielo
Copy link
Contributor

No description provided.

@pzygielo pzygielo marked this pull request as ready for review April 19, 2022 07:10
@pzygielo pzygielo marked this pull request as draft April 19, 2022 07:29
@pzygielo pzygielo force-pushed the unused branch 2 times, most recently from 9114f4a to 3ffe8f2 Compare April 19, 2022 08:28
@pzygielo pzygielo marked this pull request as ready for review April 19, 2022 08:51
Copy link
Contributor

@arjantijms arjantijms left a comment

Choose a reason for hiding this comment

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

Just one comments, in many projects the latest copyright is put on top as the first line. Thought I don't think we have any conventions for this.

@pzygielo
Copy link
Contributor Author

Just one comments, in many projects the latest copyright is put on top as the first line. Thought I don't think we have any conventions for this.

I think chronological order is better in this section, so I follow already established examples

#
# Copyright (c) 2010, 2018 Oracle and/or its affiliates. All rights reserved.
# Copyright (c) 2021 Payara Services Ltd.
#

/*
* Copyright (c) 2010, 2018 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019 Payara Foundation and/or its affiliates. All rights reserved.
*

Copyright (c) 2010, 2018 Oracle and/or its affiliates. All rights reserved.
Copyright (c) 2019, 2020 Payara Services Ltd.

@dmatej
Copy link
Contributor

dmatej commented Apr 19, 2022

This discussion repeats - I had the same thoughts as you, @pzygielo , but the issue was that the copyright plugin takes the first line. Also I found that in many projects is the newer year first, probably for the same reason, so finally I had to agree with Arjan.

And another issue - using comma between years is enumerating years in which something new was added. I would prefer rather the minus character, range. Then it would be perfect :-D (but currently I don't know how copyright plugin works with that - last changes I did just added the line with 2022)

@pzygielo
Copy link
Contributor Author

This discussion repeats - I had the same thoughts as you, @pzygielo , but the issue was that the copyright plugin takes the first line.

This should be resolved in the plugin.
It is not used here though.

And another issue - using comma between years is enumerating years in which something new was added.

https://www.eclipse.org/projects/handbook/#ip-copyright-headers:

The date may optionally be set to a range of years with the first and last years of the range separated by a comma (e.g. “2004, 2017”); in this case, the first year is when the content was first created and the last year is when the content was last modified.

@dmatej
Copy link
Contributor

dmatej commented Apr 19, 2022

The date may optionally be set to a range of years with the first and last years of the range separated by a comma (e.g. “2004, 2017”); in this case, the first year is when the content was first created and the last year is when the content was last modified.

Hmm, I see this rule as quite problematic and unusual. But it probably doesn't change anything (meaning in relation to law).

@arjantijms
Copy link
Contributor

This should be resolved in the plugin.

It probably should, too, but at the same time it's still good to have a somewhat standard way of doing this.

Anyway, it was a just a remark. Don't let it hold up the merge. Just something to think about.

@arjantijms arjantijms merged commit e4550c8 into eclipse-ee4j:master Apr 19, 2022
@pzygielo pzygielo deleted the unused branch April 19, 2022 14:22
@arjantijms arjantijms added this to the 3.0.4 milestone Mar 28, 2023
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.

3 participants