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

CommunicationParameterRef: add a _build_odxlinks() method #134

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

andlaus
Copy link
Collaborator

@andlaus andlaus commented Jun 2, 2023

this allows to get rid of some special treatment of comparams in the diaglayer. (i.e., all objects featured by DiagLayer which have a ._resolve_references() should now also have .build_odxlinks())

Andreas Lauser <andreas.lauser@mercedes-benz.com>, on behalf of MBition GmbH.
Provider Information

this allows to get rid of some special treatment of comparams in the
diaglayer. (i.e., all objects featured by `DiagLayer` which have a
`._resolve_references()` should now also have `.build_odxlinks()`)

Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io>
Signed-off-by: Gerrit Ecke <gerrit.ecke@mbition.io>
@andlaus andlaus requested a review from kayoub5 June 2, 2023 07:45
@kayoub5 kayoub5 merged commit b78ccc6 into mercedes-benz:main Jun 2, 2023
6 checks passed
@andlaus
Copy link
Collaborator Author

andlaus commented Jun 2, 2023

thanks for merging. I was quite a bit confused by the fact that you used a "rebase merge" since I thought I accidentally pushed this directly to main. For this reason, I think it would be good to use the "create merge commit" or "squash and merge" strategies for future PRs. (Come think of it, you might have used the latter one, but since there was nothing to squash, it probably just pushed unmodified patch of the PR...)

Did I miss a drawback of using merge commits (besides the messier history)?

@kayoub5
Copy link
Collaborator

kayoub5 commented Jun 2, 2023

I just used the merge button default settings.

the choice of merge strategy depends on how messy the pr history is
messy pr history : discard it by squashing
clean pr history: merge commit or rebase

@andlaus
Copy link
Collaborator Author

andlaus commented Jun 2, 2023

I just used the merge button default settings.

weird. maybe it remembers what you used the last time. Anyway, not a big deal; I was quite confused, but to a large extend this was because I did not expect this...

the choice of merge strategy depends on how messy the pr history is
messy pr history : discard it by squashing
clean pr history: merge commit or rebase

+1. that is my sentiment as well. I suppose it comes down to a github usability issue then...

@andlaus andlaus deleted the comparam_build_odxlinks branch December 7, 2023 13:30
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

2 participants