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

Refactor classes into their own files #318

Merged
merged 1 commit into from
Jun 22, 2023
Merged

Conversation

cjbarth
Copy link
Contributor

@cjbarth cjbarth commented Jun 21, 2023

This moves some code around so that classes, or closely related classes, are in their own files.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #318 (a320970) into master (41502e9) will decrease coverage by 0.12%.
The diff coverage is 70.74%.

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
- Coverage   81.67%   81.56%   -0.12%     
==========================================
  Files           5        7       +2     
  Lines         824      819       -5     
==========================================
- Hits          673      668       -5     
  Misses        151      151              
Impacted Files Coverage Δ
lib/signature-algorithms.js 58.06% <58.06%> (ø)
lib/hash-algorithms.js 65.21% <65.21%> (ø)
lib/utils.js 90.10% <84.21%> (-7.81%) ⬇️
lib/signed-xml.js 87.46% <100.00%> (+4.90%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

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

lgtm. Some concerns over class naming: rather than strict TitleCase, I would prefer that we respect the casing for acronyms, ie, RSASHA1 instead of RsaSha1. This not only recognises that we are dealing with an acronym here (ie, Secure Hash Algorithm rather than Sha), but also provides an easier upgrade path.

@djaqua may also want to comment with respect to naming conventions

@cjbarth
Copy link
Contributor Author

cjbarth commented Jun 22, 2023

It makes it harder to read when multiple acronyms are in a row if we maintain all caps. As for migration, they shouldn't be using these classes anyway, they should be using strings like http://www.w3.org/2001/04/xmldsig-more#rsa-sha256, which I might also add, are lower-case and delineated (albeit by a dash instead of a capital).

The following guidelines all say to do it this way:

This is a contrary example, but no one likes it (see https://stackoverflow.com/a/45860122/271351)

Which is easier to read XmlHttpRequest or XMLHTTPRequest? Since RADAR is also an acronym, should we name a property like UFORADARID or UfoRadarId?

Copy link
Collaborator

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

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

Point taken, no further objections!

@cjbarth cjbarth merged commit e11a9d8 into node-saml:master Jun 22, 2023
7 of 9 checks passed
@cjbarth cjbarth deleted the split-files branch June 23, 2023 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants