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

Extract authenticators to their own files #22

Merged
merged 3 commits into from
May 5, 2021
Merged

Conversation

nevans
Copy link
Collaborator

@nevans nevans commented Apr 28, 2021

I've started down the path of implementing #10. This PR simply copies authenticators with some minor changes, but this raises a big issue I've been meaning to ask about: SASL. I'll open another issue to discuss that: #23. I'm marking this as a draft, pending that discussion.

In addition to moving the authenticators to new files, I've updated the rdoc and added authnid support for PLAIN.

Also updates rdoc with SASL specifications and deprecations.  Of these
four, only `PLAIN` isn't deprecated!

+@@Authenticators+ was changed to a class instance var
+@Authenticators+.  No one should have been using the class variable
directly, so that should be fine.
* Add authzid support
* must not contain NULL chars
* improve rdoc
Added RFC links to all SASL mechanism specifications.
@nevans nevans requested review from hsbt and shugo April 28, 2021 22:55
@nevans nevans marked this pull request as draft April 28, 2021 23:09
Copy link
Member

@shugo shugo left a comment

Choose a reason for hiding this comment

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

It looks good to me.
@hsbt Do you have any considerations in terms of Ruby releases?

@hsbt
Copy link
Member

hsbt commented Apr 30, 2021

👍 It seems good practice for organizing code.

@nevans Can you also move net-imap.gemspec to under the net/imap dir?

@nevans
Copy link
Collaborator Author

nevans commented Apr 30, 2021

@nevans Can you also move net-imap.gemspec to under the net/imap dir?

If we move the gemspec, won't that break existing build scripts? In particular, it means that people won't be able to use the common pattern: gem 'net-imap', github: "ruby/net-imap", ref: .... They could add glob: 'lib/net/imap/*.gemspec' but that's a fairly uncommon option, especially when there's only one gem in the repo.

@nevans
Copy link
Collaborator Author

nevans commented Apr 30, 2021

And... I only just discovered that bundler's default glob is "{,*,*/*}.gemspec". I'd assumed it was different, matching less. Still, that means the gemfile would need to be in the repo root or one dir down. Neither lib/net/imap/net-imap.gemspec nor net/imap/net-imap.gemspec would work. Am I misunderstanding something? 🙂

@nevans nevans merged commit 53ff4b0 into master May 5, 2021
@nevans nevans deleted the extract-authenticators branch May 5, 2021 20:13
@hsbt
Copy link
Member

hsbt commented May 5, 2021

f we move the gemspec, won't that break existing build scripts?

Ah, Sorry. I meant it in ruby/ruby repo. There is no action with this repo.

@hsbt
Copy link
Member

hsbt commented May 6, 2021

I tweaked sync script at ruby/ruby@5de6f1a

@nevans nevans added the SASL 🔒 Authentication and authentication mechanisms label Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SASL 🔒 Authentication and authentication mechanisms
Development

Successfully merging this pull request may close these issues.

3 participants