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.

Sign up for GitHub

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

Jump to bottom

Support package level annotations #5

Merged
merged 3 commits into from
Sep 16, 2019
Merged

Support package level annotations #5

merged 3 commits into from
Sep 16, 2019

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Jun 13, 2018 •
edited
Loading

see jenkinsci/configuration-as-code-plugin#278 for initial discussion

see jenkinsci/lib-access-modifier#13 for actual usage

@ndeloof
Support package level annotations
f913f1a 
Signed-off-by: Nicolas De Loof <[email protected]>
jetersen
jetersen approved these changes Jun 13, 2018
View reviewed changes
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

👍

@ndeloof ndeloof mentioned this pull request Jun 13, 2018
Closed
@ndeloof
collect package in index
d912d70 
Signed-off-by: Nicolas De Loof <[email protected]>
@ndeloof
Copy link
Contributor Author

ndeloof commented Jun 13, 2018 •
edited
Loading

made a minor change as this makes access-modifier easier to implement:
package-info annotations are indexed by package name (nor listing classes in package)

Drawback doing so is that such entries in index will produce warning: class file not found when an older version of access-modifier is used (but won't break).
@oleg-nenashev @Casz wdyt ?

oleg-nenashev
oleg-nenashev approved these changes Jun 14, 2018
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

looks acceptable even with such warning

jetersen
jetersen approved these changes Jun 14, 2018
View reviewed changes
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

Seems fine 👍

@ndeloof
Copy link
Contributor Author

ndeloof commented Jun 14, 2018

@oleg-nenashev can you merge (and release) this or do you expect more reviews ?

@oleg-nenashev
Copy link
Member

@jglick as the last person to release it, could you please also take a look?

jglick
jglick previously requested changes Jun 15, 2018
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Actual behavior can be tested like this. Probably you want to syntactically distinguish package names in the index file, e.g.

some.pkg.*

src/main/java/org/jvnet/hudson/annotation_indexer/AnnotationProcessorImpl.java Outdated Show resolved Hide resolved
@ndeloof
use ‘com.foo.*’ for packages
1952a83 
Signed-off-by: Nicolas De Loof <[email protected]>
@ndeloof
Copy link
Contributor Author

ndeloof commented Jun 25, 2018

@jglick can you please reconsider your review with latest changes ?

@ndeloof
Copy link
Contributor Author

ndeloof commented Jul 18, 2018

@jglick ping

@jglick jglick self-requested a review January 22, 2019 19:37
@jglick jglick dismissed their stale review January 22, 2019 19:37

possibly stale

oleg-nenashev
oleg-nenashev approved these changes Jun 18, 2019
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

is it still actual @ndeloof ? Looks so

@ndeloof
Copy link
Contributor Author

ndeloof commented Sep 6, 2019

inactive, feel free to take over

@ndeloof ndeloof closed this Sep 6, 2019
@oleg-nenashev
Copy link
Member

I will take it over, thanks!

@oleg-nenashev oleg-nenashev reopened this Sep 6, 2019
@oleg-nenashev oleg-nenashev self-assigned this Sep 6, 2019
@ndeloof
Copy link
Contributor Author

ndeloof commented Sep 8, 2019

@oleg-nenashev could you please cherry-pick those commit into your own fork so I don't get notified on this PR, I also plan to cleanup my hundreds jenkins-* forks
thanks

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Sep 8, 2019 via email

@ndeloof
Copy link
Contributor Author

ndeloof commented Sep 16, 2019

@oleg-nenashev did you pulled my commits ?

@oleg-nenashev oleg-nenashev merged commit cbac230 into jenkinsci:master Sep 16, 2019
@oleg-nenashev
Copy link
Member

I just got it merged. Should be good enough for now, thanks!

@timja timja mentioned this pull request Jan 31, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@jetersen jetersen jetersen approved these changes

@jglick jglick Awaiting requested review from jglick

Assignees

@oleg-nenashev oleg-nenashev

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ndeloof @oleg-nenashev @jglick @jetersen