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

Linked values, line breaks issue and naming fixes #63

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ideas-into-software
Copy link

  • Added support for linked values, i.e. linking cell value to a sheet, file, URI or URL

  • Fixed bug with cell values which contain line separators - setting those via both office:string-value attribute and text:p element results in office:string-value taking precedence and text displayed containing no line breaks

  • Added BND instruction to include sources in OSGi jar

  • Fixed spelling mistake in OdsWriter naming

  • Bumped version to 1.5.3

… those via both 'office:string-value' attribute and 'text:p' element results in 'office:string-value' taking precedence and text displayed containing no line breaks

 - Added BND instruction to include sources in OSGi jar

 - Fixed spelling mistake in 'OdsWriter' naming

 - Bumped version to 1.5.3
@miachm
Copy link
Owner

miachm commented Mar 26, 2023

Thanks!

I am confused with the 2 PR... So is this the right one?

@ideas-into-software
Copy link
Author

Hi Miguel,

Yes, this (#63) is the PR I would ask you review and merge.

The other PR (#62) was closed in favor of this PR, which contains both (bug fixes as well as support for linked values).

Thanks!

I am confused with the 2 PR... So is this the right one?

Copy link
Owner

@miachm miachm left a comment

Choose a reason for hiding this comment

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

Thanks for the work, it seems you spent a considerable amount to implement this feature.

I still not sure how this feature works though. Is some kind of hyperlink to another cell? Did I understand right?

Also the feature is not finished. I miss the part of ODSReader for reading a file with this feature.

@@ -6,7 +6,7 @@
<groupId>com.github.miachm.sods</groupId>
<artifactId>SODS</artifactId>
<packaging>jar</packaging>
<version>1.5.2</version>
<version>1.5.3</version>
Copy link
Owner

Choose a reason for hiding this comment

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

We use the next format in SODS for versioning:

X.Y.Z

X refers to API versions. Changes within the same X are compatible. The API of the version 1.1.0 is completely compatible with 1.5.0 (although not the other way around). The day we redesign the API, we need to update the X.
Y refers to additional features. There are aditional features in the API, although the API is still backwards-compatible.
Z refers to a bugfixing release. There are no aditional features or changes in the API

This is a new feature. Therefore, we need to update the Y version.

@@ -1,7 +1,7 @@
package com.github.miachm.sods;

import java.time.LocalDate;
import java.util.Objects;
import java.util.*;
Copy link
Owner

Choose a reason for hiding this comment

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

Why use an asterisk here?

this.value = value;
}

public static LinkedValue.Builder builder() {
Copy link
Owner

Choose a reason for hiding this comment

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

This is the builder pattern. Althought I understand the value of the pattern, is not consistent with the rest of API. SODS usually uses a more direct-straightforward style to the user.

}

public LinkedValue.Builder href(Sheet sheet) {
this.href = '#' + sheet.getName() + ".A1";
Copy link
Owner

Choose a reason for hiding this comment

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

Why A1?

*/
public class LinkedValue {
private final String href;
private final String value;
Copy link
Owner

Choose a reason for hiding this comment

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

Why a value? I thought this feature was for linking a cell to another.

Choose a reason for hiding this comment

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

Hi Miguel,

To address your comments:

  • You must have missed the fact that version was bumped with first commit, where bug fix was made; I am quite familiar with semantic versioning (https://semver.org/), I work with OSGi on a daily basis; you are free to bump it to whatever version you'd like; I needed to build the jar and include in local BND repo so I can continue on a different project where SODS library is consumed;
  • As it is mentioned in Javadoc of LinkedValue class, it is used to link a value to a sheet, file, URI or URL; this is a feature of ODS spec, perhaps you should check the generated XML code and/or generate an ODS file and open it with e.g. LibreOffice and you'll see how clicking a cell value with take you to another sheet, etc.;
  • I only need write capability, therefore I only focused on that; I do not really have the time to contribute more to this project at this point - that is all that is needed in a different project which consumes SODS library; if you or anyone else feels the need to add read capability, you are free to do so;
  • Separate class as well as builder pattern in LinkedValue was done on purpose, with hope that with time abstractions for other elements can also be created, a la https://github.com/jferard/fastods/, which is designed really well, unfortunately uses prohibitive license; currently, in SODS, everything is crammed together in ODSWriter, the elements do really know their XML representation, etc.;
  • A1 is for compatibility with Excel, where sheet links written as #SheetName do not work in Excel, but #SheetName.A1 work in both Excel and LibreOffice;
  • I am not really sure how to answer the asterisk question - I worked on this using IDEA IDE, and this is how it optimizes imports;
  • I did not have a timer on, but somewhere around 4+ hours, including time needed to analyze and come up with solution, plus additional integration test cases;

Feel free to use this as you'd like, I know I am already using this in a project where SODS is consumed. Sorry I do not have more time to help now.

Copy link
Owner

Choose a reason for hiding this comment

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

Don't get me wrong, I am grateful for any contribution. My comments try to balance between being appreciative and giving meaningful feedback according to the library standards. But sometimes I can excesively direct. I understand not everyone have the time to fully contribute to the development (dam, I don't even have the time myself).

Yep, some classes in SODS are becoming in god classes, specially ODSReader/ODSWritter like you mentioned. Nevertheless I try to keep the external-user API as consistent as posible, that's why the build pattern does not fit with the current implementation, because it presents to the user a completely different style. Eventually I'll need to deprecate the API and re-design it, but, you know, not enough time!

Thanks again, let's see if I can finish the feature and release another version.

@juergen-albert
Copy link

@miachm does anything speaks against merging the current state of this? We'd really like to use this and it would be great if we could just use a Version from maven Central instead of one from our own fork.

@miachm
Copy link
Owner

miachm commented Jan 17, 2024

@juergen-albert I think I just forgot about it. Now it need to fix the conflicts t.t, but yes, it can be added to the next version.

@juergen-albert
Copy link

This is highly appreciated. If you need a hand, just reach out.

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.

3 participants