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.

Sign up for GitHub

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

Jump to bottom

[LTB-39] Reorganise HasLens to make naming more predictable #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[LTB-39] Reorganise HasLens to make naming more predictable #28

wants to merge 1 commit into from

Conversation

kirelagin
Copy link
Contributor

Please, see the commit message for details.

Do we really need the tagged versions of these classes?

@kirelagin
LTB-39 Reorganise HasLens to make naming more predictable
26f7b12 
* Bring the naming of `HasLens` & Co into line with the `lens` lib:
    * Rename weird `HasLens` to `HasTaggedLens`
    * Add `HasLens` that can change the type
* Add `HasTaggedGetter` and similarly `HasGetter`
* Create a relationship between `HasLens` and `HasGetter`
  (probably we should add `HasSetter` as well, make it a superclass
  of `HasLens` with a default implementation and then provide a
  default implementation for `HasLens` in terms of the other two).

This change breaks Log and Network. Fixes for them are coming in a
future commit.
@kirelagin kirelagin changed the title LTB-39 Reorganise HasLens to make naming more predictable [LTB-39] Reorganise HasLens to make naming more predictable Aug 14, 2018
volhovm
volhovm suggested changes Aug 14, 2018
View reviewed changes
Copy link
Contributor

@volhovm volhovm left a comment

Choose a reason for hiding this comment

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

Who'll be fixing the compatibility issues all over everywhere?

@volhovm
Copy link
Contributor

volhovm commented Aug 14, 2018

We don't really need the tagged version, and I doubt we actually need this PR at all.

@kirelagin
Copy link
Contributor Author

kirelagin commented Aug 14, 2018 •
edited

Who'll be fixing the compatibility issues all over everywhere?

Please, see the commit message for details.

I doubt we actually need this PR at all

We might not need the lens change, it is only there to reduce the confusion of naming, but we definitely do need HasGetter. Requiring a lens is too strong a requirement, you only need a Getter 80% of the time (e.g. in most places in loot-log and in every single place in loot-network).

@Martoon-00
Copy link
Member

Martoon-00 commented Sep 7, 2018 •
edited

Crying that HasGetter is not yet in master

I agree that we'll barely have use for tags in disciplina ever. And don't like this PR only because it changes s/HasCtx/HasCtx', and I got tired of these primes everywhere.

I'm ready to resolve compatibility issues once this is merged.

@Martoon-00 Martoon-00 closed this Sep 7, 2018
@serokell-bot serokell-bot deleted the kirelagin/ltb39-haslens branch September 7, 2018 13:40
@Martoon-00 Martoon-00 restored the kirelagin/ltb39-haslens branch September 7, 2018 13:41
@Martoon-00 Martoon-00 reopened this Sep 7, 2018
@Martoon-00
Copy link
Member

@kirelagin Don't you mind if instead of s/HasLens/HasTaggedLens renaming I remove the tag parameter?

@kirelagin
Copy link
Contributor Author

I think, I don’t mind. It might be there only because I couldn’t make things work without it. I honestly don’t remember.

@Martoon-00
Copy link
Member

Martoon-00 commented Dec 10, 2018 •
edited

It might be there only because I couldn’t make things work without it

You mean within lootbox?

@Martoon-00
Copy link
Member

I suppose I'll try to do that, if nothing breaks within lootbox I will assume it is alright change.

@kirelagin
Copy link
Contributor Author

You mean within lootbox?

I think so, yes, but I’m not sure.

@Martoon-00 Martoon-00 mentioned this pull request Dec 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@volhovm volhovm volhovm requested changes

@Martoon-00 Martoon-00 Awaiting requested review from Martoon-00

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kirelagin @volhovm @Martoon-00