refactor: Trim imported connor package #530

AndrewSisley · 2022-06-15T20:50:55Z

Relevant issue(s)

Resolves #529

Description

Simplifies the connor package a little bit. Done whilst waiting on reviewers for #471

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Integration tests.

Specify the platform(s) on which this was tested:

Debian Linux

codecov · 2022-06-15T20:51:02Z

Codecov Report

Merging #530 (e3bc9f5) into develop (3957381) will increase coverage by 0.00%.
The diff coverage is 89.79%.

@@            Coverage Diff            @@
##           develop     #530    +/-   ##
=========================================
  Coverage    56.58%   56.59%            
=========================================
  Files          121      118     -3     
  Lines        14082    13952   -130     
=========================================
- Hits          7969     7896    -73     
+ Misses        5421     5364    -57     
  Partials       692      692

Impacted Files	Coverage Δ
query/graphql/mapper/targetable.go	`87.50% <ø> (ø)`
query/graphql/parser/filter.go	`62.06% <ø> (-0.96%)`	⬇️
connor/and.go	`58.33% <50.00%> (-10.42%)`	⬇️
connor/in.go	`58.33% <50.00%> (-10.42%)`	⬇️
connor/or.go	`58.33% <50.00%> (-10.42%)`	⬇️
connor/connor.go	`92.30% <92.00%> (+46.85%)`	⬆️
connor/eq.go	`80.55% <100.00%> (+23.65%)`	⬆️
connor/ge.go	`66.66% <100.00%> (+16.66%)`	⬆️
connor/gt.go	`66.66% <100.00%> (+16.66%)`	⬆️
connor/le.go	`66.66% <100.00%> (+16.66%)`	⬆️
... and 8 more

AndrewSisley · 2022-06-15T20:54:27Z

I might continue to grow this whilst waiting on #471 so maybe dont rush to review this or anything

fredcarle · 2022-06-17T00:04:23Z

praise: I like this a lot! I'll review more later but on first pass it's 👌

We dont need to worry about this being called outside of this lib

I think this simplifies the code futher, feel very free to contradict me. Inits had to be preserved as Golang doesnt seem to treat static functions as constants and complained about an infinate init loop

No need for runtime string matching etc

No need to bother with prefix, so long as it wont panic any unknown operators will be caught on the next few lines.

Allows removal of prefix swap, and (I think) makes the explain results more user friendly.

We will probably add this back in at somepoint, but it is easy to do so and at the moment it is dead code

fredcarle

LGTM!

shahzadlone

This is such a beautiful trim. Thanks for taking the time to clean this up! I had two non-blocking thoughts/questions/suggestions:

Perhaps some documentation on the only functions that are remaining to be documented under the connor package for completeness in the following files:
- connor/key.go
- connor/numbers/upcast.go
- connor/numbers/equality.go
Since this seems to be the PR where we start "owning" connor did we want to do the rename to eval here? (or is that planned for a follow-up PR?)
Why not add the license on the headers of the files like we have for net? have we decided to have this entire folder be MIT from now on?

AndrewSisley · 2022-06-28T21:55:47Z

Cheers Shahzad, answers inline below:

Perhaps some documentation...

Will look into this, but from memory upcast and equality haven't really been touched by me, and I might be unqualified to do so, so no promises

expand docs?

did we want to do the rename connor to eval here?

I would suggest this is out of scope, I see this as a pure technical refactoring to make the code simpler for us, and I don't particularly care what the package is called. Might also be nice to do this in it's own PR to ensure a clear commit (given that we are not the original authors.

Why not add the license on the headers...?

Similar to (2), I don't really want to deal with that here. Is a technical PR only in my eyes, and I'm not so comfy changing the license, and would probably want a PR specific to that for clearer commit-tracking (if we do want to change the licence).

shahzadlone · 2022-06-28T22:33:38Z

Cheers Shahzad, answers inline below:

Perhaps some documentation...

Will look into this, but from memory upcast and equality haven't really been touched by me, and I might be unqualified to do so, so no promises

expand docs?

did we want to do the rename connor to eval here?

I would suggest this is out of scope, I see this as a pure technical refactoring to make the code simpler for us, and I don't particularly care what the package is called. Might also be nice to do this in it's own PR to ensure a clear commit (given that we are not the original authors.

Why not add the license on the headers...?

Similar to (2), I don't really want to deal with that here. Is a technical PR only in my eyes, and I'm not so comfy changing the license, and would probably want a PR specific to that for clearer commit-tracking (if we do want to change the licence).

Cheers, thanks for the clarifications. Let'ss ssship it, or should I say trimmmm itttt hahaha.

orpheuslummis · 2022-06-28T23:07:18Z

suggestion: replace the README (already deleted) by package documentation demonstrating its remaining functionality.

orpheuslummis · 2022-06-28T23:09:30Z

Similar to (2), I don't really want to deal with that here. Is a technical PR only in my eyes, and I'm not so comfy changing the license, and would probably want a PR specific to that for clearer commit-tracking (if we do want to change the licence).

I saw it not as changing the license but just implementing the convention of indicating the license as a header, so instead of having the folder-level license file, each file having a header indicating its license.

IMO if we agree on following that convention, might as well do that here and not summon another PR just for that.

AndrewSisley · 2022-06-29T12:47:09Z

IMO if we agree on following that convention, might as well do that here and not summon another PR just for that.

I am of the opinion that it would be more efficient to merge this now instead of waiting on consensus here (let alone dev time to make any potiential move)

AndrewSisley · 2022-06-29T12:50:37Z

suggestion: replace the README (already deleted) by package documentation demonstrating its remaining functionality.

Now that this is internal code, and how we have a fair number of integration tests covering aspects of this that we use (providing lots of examples), I would suggest that this is a 'nice to have' and doesn't quite justify the dev time atm. Is also as easy to add later than to add now.

AndrewSisley · 2022-06-29T12:54:59Z

Will look into this, but from memory upcast and equality haven't really been touched by me, and I might be unqualified to do so, so no promises

Looked at this again today, upcast and equality look pretty self explanatory, and key is already documented. Leaving as is.

I wonder if there is a very tiny performance cost to doing this, but I like the removal of the extra code branch and possible error.

* Remove unused case Could be re-added in some form when adding filters for inline-arrays perhaps * Our filters can never contain these types (always .*64) * We dont support these operators for strings * Make MatchWith private We dont need to worry about this being called outside of this lib * Remove unused Operators func * Reduce connor state I think this simplifies the code futher, feel very free to contradict me. Inits had to be preserved as Golang doesnt seem to treat static functions as constants and complained about an infinate init loop * Simplify condition recursion No need for runtime string matching etc * Simplify operator safety check No need to bother with prefix, so long as it wont panic any unknown operators will be caught on the next few lines. * Use '_' as operator prefix. Allows removal of prefix swap, and (I think) makes the explain results more user friendly. * Remove unused fields sub package * Remove unfriendly test suite stuff * Remove out of date readme * Remove unsupported time checks We will probably add this back in at somepoint, but it is easy to do so and at the moment it is dead code * Remove unsupported contains operator * Replace map, init funcs, and type, with simple switch * Dont bother skipping first rune in switch I wonder if there is a very tiny performance cost to doing this, but I like the removal of the extra code branch and possible error. * Stop looping on eq failure

AndrewSisley added area/query Related to the query component refactor This issue specific to or requires *notable* refactoring of existing codebases and components priority/low labels Jun 15, 2022

AndrewSisley requested review from orpheuslummis, jsimnz, fredcarle, Lodek and shahzadlone June 15, 2022 20:50

AndrewSisley self-assigned this Jun 15, 2022

AndrewSisley changed the base branch from develop to lone/rebased/sisley/refactor/I448-doc-restructure June 15, 2022 20:51

AndrewSisley changed the base branch from lone/rebased/sisley/refactor/I448-doc-restructure to sisley/refactor/I448-doc-restructure June 15, 2022 20:51

AndrewSisley added the action/no-benchmark Skips the action that runs the benchmark. label Jun 15, 2022

AndrewSisley force-pushed the sisley/refactor/I448-doc-restructure branch from 3a32d83 to 24744cd Compare June 15, 2022 21:06

AndrewSisley force-pushed the sisley/refactor/I529-connor-trim branch from 1a9ba41 to 9b78eab Compare June 15, 2022 21:08

AndrewSisley mentioned this pull request Jun 16, 2022

refactor: Internal doc restructure #471

Merged

26 tasks

AndrewSisley force-pushed the sisley/refactor/I529-connor-trim branch from 7d97edd to ce9790c Compare June 16, 2022 15:25

AndrewSisley force-pushed the sisley/refactor/I448-doc-restructure branch 11 times, most recently from 9ef3540 to 2f2ebc8 Compare June 17, 2022 16:29

AndrewSisley added 12 commits June 28, 2022 09:49

We dont support these operators for strings

c60625c

Make MatchWith private

69a245b

We dont need to worry about this being called outside of this lib

Remove unused Operators func

7a38fd1

Reduce connor state

c73d827

I think this simplifies the code futher, feel very free to contradict me. Inits had to be preserved as Golang doesnt seem to treat static functions as constants and complained about an infinate init loop

Simplify condition recursion

94ef2ce

No need for runtime string matching etc

Simplify operator safety check

de1fe20

No need to bother with prefix, so long as it wont panic any unknown operators will be caught on the next few lines.

Use '_' as operator prefix.

17325ff

Allows removal of prefix swap, and (I think) makes the explain results more user friendly.

Remove unused fields sub package

aae12fc

Remove unfriendly test suite stuff

0c5df11

Remove out of date readme

d268b91

Remove unsupported time checks

fa501c7

We will probably add this back in at somepoint, but it is easy to do so and at the moment it is dead code

Remove unsupported contains operator

c172011

AndrewSisley force-pushed the sisley/refactor/I529-connor-trim branch from ce9790c to c539992 Compare June 28, 2022 13:50

AndrewSisley marked this pull request as ready for review June 28, 2022 13:51

fredcarle approved these changes Jun 28, 2022

View reviewed changes

shahzadlone approved these changes Jun 28, 2022

View reviewed changes

AndrewSisley added 3 commits June 29, 2022 08:55

Replace map, init funcs, and type, with simple switch

7c32702

Dont bother skipping first rune in switch

074202f

I wonder if there is a very tiny performance cost to doing this, but I like the removal of the extra code branch and possible error.

Stop looping on eq failure

e3bc9f5

AndrewSisley force-pushed the sisley/refactor/I529-connor-trim branch from c539992 to e3bc9f5 Compare June 29, 2022 12:56

AndrewSisley merged commit 6ea137c into develop Jun 29, 2022

AndrewSisley deleted the sisley/refactor/I529-connor-trim branch June 29, 2022 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Trim imported connor package #530

refactor: Trim imported connor package #530

AndrewSisley commented Jun 15, 2022

codecov bot commented Jun 15, 2022 •

edited

Loading

AndrewSisley commented Jun 15, 2022

fredcarle commented Jun 17, 2022

fredcarle left a comment

shahzadlone left a comment •

edited

Loading

AndrewSisley commented Jun 28, 2022 •

edited

Loading

shahzadlone commented Jun 28, 2022

orpheuslummis commented Jun 28, 2022 •

edited

Loading

orpheuslummis commented Jun 28, 2022 •

edited

Loading

AndrewSisley commented Jun 29, 2022

AndrewSisley commented Jun 29, 2022

AndrewSisley commented Jun 29, 2022

refactor: Trim imported connor package #530

refactor: Trim imported connor package #530

Conversation

AndrewSisley commented Jun 15, 2022

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Jun 15, 2022 • edited Loading

Codecov Report

AndrewSisley commented Jun 15, 2022

fredcarle commented Jun 17, 2022

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone left a comment • edited Loading

Choose a reason for hiding this comment

AndrewSisley commented Jun 28, 2022 • edited Loading

shahzadlone commented Jun 28, 2022

orpheuslummis commented Jun 28, 2022 • edited Loading

orpheuslummis commented Jun 28, 2022 • edited Loading

AndrewSisley commented Jun 29, 2022

AndrewSisley commented Jun 29, 2022

AndrewSisley commented Jun 29, 2022

codecov bot commented Jun 15, 2022 •

edited

Loading

shahzadlone left a comment •

edited

Loading

AndrewSisley commented Jun 28, 2022 •

edited

Loading

orpheuslummis commented Jun 28, 2022 •

edited

Loading

orpheuslummis commented Jun 28, 2022 •

edited

Loading