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

Move OS to logic util to fix architecture test #6717

Closed
wants to merge 3 commits into from
Closed

Conversation

Siedlerchr
Copy link
Member

Fixes #6697

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 27, 2020
@Siedlerchr Siedlerchr added this to the v5.1 milestone Jul 27, 2020
@tobiasdiez
Copy link
Member

Having OS in model is ok for me, but why is the newlines formatter in model in the first place? All other cleanup ops are in logic (and belong there in my opinion). It becomes less and less clear where the boundary is. Probably something for the next devcall.

@Siedlerchr
Copy link
Member Author

It all started with a simple line check . That changes created a mountain of work in chain reaction.

String newValue = formatter.format(oldValue);
if (formatter instanceof NormalizeNewlinesFormatter) {
newValue = oldValue;
}

This introduced the original error. model cannot depend on logic. However, now I am in the bad situation that those Cleanups are used in Metadata and therefore in model. So I can't move it up to logic without killing the whole model.
That's why I tried to move it down to model. At least that one Formatter. But that talks to OS.Newline and L10n. SO I had to put OS down to model as well.
Argh!

@tobiasdiez tobiasdiez removed this from the v5.1 milestone Jul 29, 2020
@koppor
Copy link
Member

koppor commented Aug 1, 2020

The referenced code relates to JabRef#390. Thus, in the concrete case, this condition is a hack (fixing the cursor jumping), but not the underlying issue.

Thus, I would add an exception to the architecture tests for the release, revert it after the release and do a real fix at JabCon.

@tobiasdiez
Copy link
Member

Surpassed by #6825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing architecture tests
4 participants