Comments (21)
Thanks for reporting this issue. If you have the time, please submit a fix.
from docjure.
Actually, on writing some tests for an upcoming patch, I found that select-columns
actually mostly works - it does skip blank rows/cells, but as it uses .getColumnIndex
to work out column mappings, it assigns the correct column labels anyway.
What is the preferred behaviour for select-columns
on a blank row/column?
- Should it return an empty map for a blank row? (I'm inclined to say yes)
- Should it return a nil map entry for a blank cell? (I'm inclined to say no)
from docjure.
With the Clojure semantics of maps, empty map for a blank row would have the same result as the latter solution in normal use cases, so I would support your recommendation, blank row = empty map.
from docjure.
PS: please note that the repo has moved, the old master still works but for good measure consider changing your master to github.com/mjul/docjure (was: github.com/ative/docjure)
from docjure.
For all those with this problem, as a quick fix you can just use with-redefs
without changing dependencies or cloning the repo:
(with-redefs [dk.ative.docjure.spreadsheet/row-seq (fn [^Sheet sheet]
(map #(.getRow sheet %) (range 0 (inc (.getLastRowNum sheet)))))]
;;load your excel file here
)
- The function code was c+p'ed from @kornysietsma.
- This only covers the row-seq case but you can use @kornysietsma's code for all other functions in a similar way: https://github.com/mjul/docjure/pull/22/files
- you will need to
(:import org.apache.poi.ss.usermodel.Sheet)
in your ns declaration
from docjure.
Just to check if the project is still alive. Ability to read sparse tables correctly is kind of a big deal and at least my experiments with this PR show it's legit and does the right thing. Be great to have it merged.
Big kudos to @kornysietsma for the fix!
Big thanks to creators and maintainers!
Note: with this fix rows of different lengths may require padding at the end. Not a big deal and probably not something you want by default but might trip someone. Only matters in cases where you deal with valid tables with some rows without data in their tail-cells.
from docjure.
@vkz I agree. If someone would update the pull req (#22) from @kornysietsma to match the latest version and add some example spreadsheets with missing rows and cells to the test suite, I would be happy to merge it.
from docjure.
I'll try to update it - hopefully it shouldn't take long, I just need
prodding :-)
On 11 Aug 2016 8:52 a.m., "Martin Jul" [email protected] wrote:
@vkz https://github.com/vkz I agree. If someone would update the pull
req (#22 #22) from @kornysietsma
https://github.com/kornysietsma to match the latest version and add
some example spreadsheets with missing rows and cells to the test suite, I
would be happy to merge it.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABFZTogQdEj-I-pHeWd68GJR-_R5HdKks5qetS3gaJpZM4Ba29W
.
from docjure.
I've been looking again at this patch, and I realised why I didn't get back
to it - it needs some decisions on how best to implement it, and my
preferred option may be a breaking change for some clients.
The problem is, ideally I'd want to just replace the existing row-seq and
cell-seq with the dense functions, so they always return nil for missing
rows / cells. However, this could easily be a breaking change if there are
clients that don't expect nil return values (for example, the docjure
'set-row-style!' function would need to be modified to ignore nil cells)
Would this be OK?
Alternatively, you could make this optional - pass an extra
missing-cell-policy flag, or set a local binding like missing-cell-policy
that specifies which approach to use, and add a macro like
(with-missing-cell-policy :return-nil ...) - but this might be needless
complication.
So, would you prefer:
- Two parallel sets of functions (as per my original patch) cell-seq and
dense-cell-seq, row-seq and dense-row-seq, users can choose which they
prefer by calling different functions - Modify cell-seq and row-seq to return nils for blank cells/rows,
possible breaking change - Let the user choose sparse or dense mode
3a. via an extra option parameter to lots of functions
3b. via a local binding
- Korny
On 11 August 2016 at 12:39, Kornelis Sietsma [email protected] wrote:
I'll try to update it - hopefully it shouldn't take long, I just need
prodding :-)On 11 Aug 2016 8:52 a.m., "Martin Jul" [email protected] wrote:
@vkz https://github.com/vkz I agree. If someone would update the pull
req (#22 #22) from @kornysietsma
https://github.com/kornysietsma to match the latest version and add
some example spreadsheets with missing rows and cells to the test suite, I
would be happy to merge it.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABFZTogQdEj-I-pHeWd68GJR-_R5HdKks5qetS3gaJpZM4Ba29W
.
Kornelis Sietsma korny at my surname dot com http://korny.info
.fnord { display: none !important; }
from docjure.
Hey - trying to close this off... :)
In the absence of responses to my previous over-long essay, I'm going to go with the slightly messy option of letting people choose which way docjure will work via a dynamic var and a wrapper macro.
That seems to be the only way to preserve current behaviour, without duplicating a whole bunch of functions, or adding extra parameters all over the place
So if you wrap code as follows:
(with-missing-values-as-nil (for [row (row-seq sheet) cell (cell-seq row)] ... )
then missing rows and cells will be returned as nil
I'm cleaning up the tests and docs, will probably get a pull request in soon.
from docjure.
Hi @kornysietsma
I think your initial initial suggestion of always returning nils for blank rows makes more sense.
That it is not backwards compatible is not a problem: it is a bug-fix, after all.
Thanks for helping out.
Cheers,
Martin
from docjure.
Not my decision but I'd also prefer option 2 - returning nil for blank rows/cells.
The reason is that removing nils from the dense version is easy with standard clojure functionality.
Adding nils to the non-dense version at the right places is impossible.
So the former is more general one but still requires only few workarounds to make it work as before.
Probably the changelog should contain the word BREAKING CHANGE and you might want to increase the major version.
from docjure.
Cool - I'll go down that route then, it'll keep the code and the tests much
simpler.
- Korny
On 6 September 2016 at 08:54, Daniel Michulke [email protected]
wrote:
Not my decision but I'd also prefer option 2 - returning nil for blank
rows/cells.
The reason is that removing nils from the dense version is easy with
standard clojure functionality
Adding nils to the non-dense version at the right places is impossible.So the former is more general one but still requires only few workarounds
to make it works a before.Probably the changelog should contain the word BREAKING CHANGE and you
might want to increase the major version.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABFZT97_KoVhiMUisrtzX3XhrwBq9yhks5qnRwhgaJpZM4Ba29W
.
Kornelis Sietsma korny at my surname dot com http://korny.info
.fnord { display: none !important; }
from docjure.
I've updated the pull request #22
from docjure.
@mjul: Thanks for a great tool. All the others here too, esp. also @kornysietsma.
On topic: I think returning nil always for blank rows and cells is the right decision. It will make row-seq and cell-seq much more dependable.
from docjure.
The fix for this issue from @kornysietsma has been included in version 1.11.0. Thanks for helping make the library better.
from docjure.
@kinleyd Thanks for the kind words. I was very happy to see that you are using it in Bhutan as it is one of my favourite countries. If you have any ideas or suggestions, don't hesitate to contribute to the project.
from docjure.
@dmichulke Thanks for the thoughtful feedback. I decided not to upgrade the major version as I consider this a bug-fix (blank lines being skipped was a thing leaking through from the underlying POI library). Please keep your suggestions and pull requests coming.
from docjure.
@vkz This has now been fixed, so please give it a whirl and submit feedback, suggestions and pull requests to make it even better.
from docjure.
@kornysietsma Thanks a lot for helping out with this. It is really good work. Please send more pull requests in the future.
from docjure.
@mjul: Nice to know you like Bhutan! docjure is a great library and I look
forward to making some contributions where I can. Keep up the good work.
On Mon, Sep 19, 2016 at 7:22 PM Martin Jul [email protected] wrote:
@kornysietsma https://github.com/kornysietsma Thanks a lot for helping
out with this. It is really good work. Please send more pull requests in
the future.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABWrvyNOCM-1J13C485_dTMtSG_OQO2Yks5qrox2gaJpZM4Ba29W
.
from docjure.
Related Issues (20)
- Generate documentation (on push) HOT 1
- Add documentation build and badge HOT 3
- Expose quotePrefixed style attribute in `create-cell-style!` HOT 2
- Reuse similar-looking CellStyles HOT 1
- New release needed to support auto-size-all-columns! documented in README.md HOT 1
- Make `dk.ative.docjure.spreadsheet/create-date-format` public
- Simplest way to place data HOT 2
- ODS file support HOT 4
- Working arond "Zip bomb detected!" HOT 1
- Error upgrading to 0.13 using java 1.8 HOT 3
- Automatically use header row to read tabular sheet HOT 1
- CVE-2019-12415 HOT 9
- Numeric text string being read as float HOT 7
- Get index with select-columns HOT 1
- Writing on an empty cell HOT 2
- SKEW formula in Excel results in exception instead of :FUNCTION_NOT_IMPLEMENTED HOT 2
- Output a formula HOT 1
- Applying styles to multiple sheets? HOT 1
- Create save to stream example in documentation
- CVE-2022-26336: org.apache.poi/poi "4.1.1" HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from docjure.