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 process routines out of saml.ts #130

Merged
merged 1 commit into from
Jul 23, 2022

Conversation

cjbarth
Copy link
Collaborator

@cjbarth cjbarth commented Jul 23, 2022

In the spirit of #19, move some functions out of the very long saml.ts to another file without breaking class semantics. This will make it easier to land PRs with less chance of conflicts going forward.

@codecov
Copy link

codecov bot commented Jul 23, 2022

Codecov Report

Merging #130 (4eb41fc) into master (20ceb86) will increase coverage by 0.30%.
The diff coverage is 79.28%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   80.51%   80.82%   +0.30%     
==========================================
  Files          12       13       +1     
  Lines         821      829       +8     
  Branches      243      242       -1     
==========================================
+ Hits          661      670       +9     
  Misses         72       72              
+ Partials       88       87       -1     
Impacted Files Coverage Δ
src/saml/process.ts 78.35% <78.35%> (ø)
src/saml.ts 82.38% <100.00%> (+1.69%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@cjbarth cjbarth merged commit fec480d into node-saml:master Jul 23, 2022
@cjbarth cjbarth deleted the process-refactor branch July 23, 2022 00:48
@cjbarth cjbarth added the chore label Jul 23, 2022
@cjbarth
Copy link
Collaborator Author

cjbarth commented Jul 23, 2022

@markstos I would like your honest opinion on these PRs (#129, #130) before I continue. Do you think this adds to, or detracts from, the maintainability, debug-ability, comprehensibility, or any other aspect of the code by breaking it up like this?

I note that in VS Code "Find All References..." no longer works as one might expect when the code is refactored into multiple files. I'm not sure if losing that is worth any other gains we might get. After seeing how this looks so far, I'm starting to see how this might not be the 100% win it seemed it might be.

@srd90, @zoellner, @sibelius, @josecolella, @gugu since you also spend a lot of time with this code, I'd value your opinion as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant