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

Aggregate Bug #79 #80

Merged
merged 4 commits into from
Jan 12, 2019
Merged

Aggregate Bug #79 #80

merged 4 commits into from
Jan 12, 2019

Conversation

gerneio
Copy link
Contributor

@gerneio gerneio commented Jan 11, 2019

Proposed fix for: #79

Untested with code in this form since I do not have a Node.js or Angular environment setup. However it was tested with alterations to the Browersified version. Check out this fiddle.

P.S. First Pull Request :)

Proposed fix for: ENikS#79

Untested with code in this form since I do not have a Node.js or Angular environment setup. However it was tested with alterations to the Browersified version (https://unpkg.com/linq-es2015@2.4.33/dist/linq.js).
@coveralls
Copy link

coveralls commented Jan 11, 2019

Coverage Status

Coverage increased (+0.0004%) to 99.804% when pulling 0369611 on gerneio:patch-1 into bb1984a on ENikS:master.

@ENikS
Copy link
Owner

ENikS commented Jan 12, 2019

Seems to have an issue with build.

@gerneio
Copy link
Contributor Author

gerneio commented Jan 12, 2019

I can't really make out whatever it's complaining about.

@ENikS
Copy link
Owner

ENikS commented Jan 12, 2019

485 lib/linq.ts(193,66): error TS2345: Argument of type 'A' is not assignable to parameter of type 'number'.

@ENikS
Copy link
Owner

ENikS commented Jan 12, 2019

Could you please explain what exactly are you doing?

TypeScript type checking
@gerneio
Copy link
Contributor Author

gerneio commented Jan 12, 2019

I think I figured it out. Also, I implemented the exact solution I proposed in the issue, which I did explain partly why it should be done like that. Although I'm sure there are better ways.

@gerneio
Copy link
Contributor Author

gerneio commented Jan 12, 2019

Awesome! Looks like the checks passed!

@ENikS
Copy link
Owner

ENikS commented Jan 12, 2019

Could you be so kind and add an extra test for the condition you are trying to fix?

@gerneio
Copy link
Contributor Author

gerneio commented Jan 12, 2019

Whats the proper way to go about that?

@gerneio
Copy link
Contributor Author

gerneio commented Jan 12, 2019

Add it to immediate.ts in test under the 'Aggregate' area?

@ENikS
Copy link
Owner

ENikS commented Jan 12, 2019

Just add it here

gerneio added a commit to gerneio/LINQ that referenced this pull request Jan 12, 2019
Test Case for ENikS#80 from issue ENikS#79
@ENikS
Copy link
Owner

ENikS commented Jan 12, 2019

Almost there

test/immediate.ts(42,70): error TS2345: Argument of type '(c: number, n: string) => string' is not assignable to parameter of type '(aggr: number, x: string) => number'.

Type 'string' is not assignable to type 'number'.

Remove Number test case. Only applicable to browserfied code.
@ENikS ENikS merged commit a02192f into ENikS:master Jan 12, 2019
@gerneio
Copy link
Contributor Author

gerneio commented Jan 12, 2019

Almost indeed! I know nothing of TypeScript, as far as coding it, but makes sense that I'm seeing these kind of errors. Browser JS is too lenient, but that's the way I like it.

I just removed one of the test cases since it was only pertinent to how the browserfied code would handle it.

Boom! Success!

@ENikS
Copy link
Owner

ENikS commented Jan 12, 2019

Thank you for your help!

@gerneio
Copy link
Contributor Author

gerneio commented Jan 12, 2019

No problem. Thanks for bearing with me. I've got my foot in the door now.

@gerneio gerneio deleted the patch-1 branch January 12, 2019 00:54
@gerneio
Copy link
Contributor Author

gerneio commented Jan 12, 2019

BTW, is that browserfied version hosted on the NPM CDN updated automatically through some linked process, or is that a manual update process? If so, any idea when it can be updated?

@ENikS
Copy link
Owner

ENikS commented Jan 12, 2019

It should be automated. Path includes the version...

@gerneio
Copy link
Contributor Author

gerneio commented Jan 12, 2019

Hmmm I feel stupid for not being able to figure this out but I've loaded it from this link
https://unpkg.com/linq-es2015@2.4.33/lib/linq.js and https://unpkg.com/linq-es2015@2.4.33/dist/linq.js, yet I still don't see the changes. I even tried just the root path (https://unpkg.com/linq-es2015) so that it would auto direct to the latest version, but same as the lib one. Thought it may have been caching it, but no difference. Perhaps I'll just check tomorrow and all will be fine!

@ENikS
Copy link
Owner

ENikS commented Jan 12, 2019

@2.4.33 - is the version

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

Successfully merging this pull request may close these issues.

None yet

3 participants