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

Fix issue in case when namespace has no prefix #330

Merged
merged 1 commit into from
Jul 8, 2023
Merged

Fix issue in case when namespace has no prefix #330

merged 1 commit into from
Jul 8, 2023

Conversation

git9527
Copy link
Contributor

@git9527 git9527 commented Jul 6, 2023

@cjbarth please check the PR

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #330 (88331e6) into master (131bcf9) will increase coverage by 0.38%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
+ Coverage   82.49%   82.88%   +0.38%     
==========================================
  Files           7        7              
  Lines         777      777              
==========================================
+ Hits          641      644       +3     
+ Misses        136      133       -3     
Impacted Files Coverage Δ
lib/c14n-canonicalization.js 67.88% <100.00%> (+3.73%) ⬆️
lib/utils.js 89.77% <100.00%> (-0.34%) ⬇️

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

@cjbarth
Copy link
Contributor

cjbarth commented Jul 6, 2023

Can you do me a favor and make a PR against this branch with these changes too? That would help prevent this code from being lost during the migration to TS.

#325

@git9527
Copy link
Contributor Author

git9527 commented Jul 8, 2023

I've loved to, but there're some error when executing npm install on your ts-convert branch
Screenshot 2023-07-08 at 15 54 36

Could you help merge these changes and perform new release? Meanwhile help me resolve the ts issues, I can sync changes there

@cjbarth
Copy link
Contributor

cjbarth commented Jul 8, 2023

Meanwhile help me resolve the ts issues, I can sync changes there

I'm working on this. It seems there are a few more types that need to be tightened up and otherwise corrected.

@cjbarth cjbarth added the bug label Jul 8, 2023
@cjbarth cjbarth merged commit ec309ec into node-saml:master Jul 8, 2023
@cjbarth cjbarth changed the title fix issue in case when namespace has no prefix Fix issue in case when namespace has no prefix Jul 8, 2023
@cjbarth
Copy link
Contributor

cjbarth commented Jul 9, 2023

@git9527 , don't worry about the compilation errors, just make a PR against #325 and I'll fix that kind of stuff when I'm working on the other stuff. I just don't want to have to fiddle with the .js -> .ts rename stuff causing a merge conflict. There is no way to automatically resolve that, so PR that would do that would be super helpful.

@git9527
Copy link
Contributor Author

git9527 commented Jul 10, 2023

@cjbarth please check cjbarth#3, could you release new version from 3.x?

@cjbarth
Copy link
Contributor

cjbarth commented Jul 10, 2023

@git9527 Thank you so much for that. #336 is the last PR waiting to land before I release 3.x. However, it is a simple PR, so I may just do it for them, since it is a cherry pick PR, so that I can unblock you.

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.

2 participants