search

fiduswriter / biblatex-csl-converter

A set of JavaScript converters: bib(la)tex => json, json => csl, and json => biblatex
GNU Lesser General Public License v3.0
34 stars 10 forks source link

Switch to TypeScript, add crossref and origdate #123

Closed DerDrodt closed 3 years ago

DerDrodt commented 3 years ago

This PR switches all source files to TypeScript, allowing easier usage of this library in other projects.

The BibLaTeX reader is now able to handle the crossrefas specified in the BibLaTeX documentation. For the definitions of the inheritance setup, see src/import/const.ts:3160.

Another added feature is support for the origdate field.

All tests have been adapted as necessary, though more are needed for crossref and origdate.

This PR is to be seen as a draft as of now, due to some types needing refining. Right now, we still rely on any and unknown a lot.

johanneswilm commented 3 years ago

@DerDrodt OK, will you work in the changes that have been merged into the package since you forked?

DerDrodt commented 3 years ago

@DerDrodt OK, will you work in the changes that have been merged into the package since you forked?

Will do!

DerDrodt commented 3 years ago

This fork should be caught up to master, featurewise.

johanneswilm commented 3 years ago

Great @DerDrodt! How about the merge conflicts and the draft status? Will you resolve the merge conflicts and notify me once you feel this is ready to be merged?

DerDrodt commented 3 years ago

I can't do much about the merge conflicts and can't even see them, but every file other than package.json is overridden anyway, so there is no problem.

As for the draft state, I still want to figure out some smarter types in a couple of places. It might take me a few days.

johanneswilm commented 3 years ago

@DerDrodt You should be able to do something like:

git remote add upstream git@github.com:fiduswriter/biblatex-csl-converter.git
git pull upstream master

Then you should see the merge conflicts locally. Once you are done resolving them, just do

git commit
git push

And that should clear everything.

I can also do it, but it would be best if you did it as you will know more about what you changed and how to resolve conflicts. @DerDrodt

DerDrodt commented 3 years ago

I fixed the merge issues and the mistakes I made while fixing, of course, but now there is the problem that the tool I use for creating the type file requires a node version >= 12. The CI uses 10. Whether you want to require v12 for this library or want me to find another tool is up to you. But node 10 is only officially supported until tomorrow anyway.

johanneswilm commented 3 years ago

@DerDrodt Yes, please upgrade node then.

One thing that would be good to revert is the indentation in the json files. If we can have two space indentations then it's easier to compare with the previous files and see whether any of the test output has changed.

johanneswilm commented 3 years ago

@DerDrodt Alternatively, we can switch the json files in the existing master branch to use 4 space indents. That may be more consistent given that we use 4 space everywhere else. I can go ahead and do that now.

johanneswilm commented 3 years ago

@DerDrodt I have adjusted the json files to have the same indent space count as your patch. Please pull one more time from master. This should make your PR smaller and also make it clearer whether there are any differences in the output.

DerDrodt commented 3 years ago

Okay, I pulled from master. It may also be a good idea to set up a formatter for cases like this. Prettier, perhaps? I'll create a PR after this is done.

johanneswilm commented 3 years ago

@DerDrodt I made another few spacing fixes in the master branch to get closer to your code. Could you pull again?

You have a newline at the end of json files. That looks to be more correct than what we previously had [1].

I would also appreciate it if @retorquere could review this PR. He knows much more about both Typescript and BibTeX than me and it would be good to hear his opinion as well on this.

[1] https://github.com/serverless/serverless/issues/1321

DerDrodt commented 3 years ago

I pulled again, there are no more conflicts.

Since I am no TypeScript expert, I don't think I will be able to improve the types much.

retorquere commented 3 years ago

I would also appreciate it if @retorquere could review this PR. He knows much more about both Typescript and BibTeX than me and it would be good to hear his opinion as well on this.

I'm not an expert on TS, just a convinced user, I'd have to get acquainted with the codebase, and this patch seems to touch in essence every single line of code -- I don't think I can be of much help here.

DerDrodt commented 3 years ago

I just remembered that I also changed one aspect of the API: Instead of an async option, there is a separate method for async parsing, parseAsync. That's easier for types. Otherwise, you can't just use the result of parse without casting it.

johanneswilm commented 3 years ago

ok, so three non-TS expert looking at a TS PR. Hmm...

The renaming of all files from .js to .ts means that the diffing mechanism doesn't help us very much on those. Not sure if we can somehow avoid that.

@retorquere How about just comparing the output files from the tests and how those have changed?

I just remembered that I also changed one aspect of the API: Instead of an async option, there is a separate method for async parsing, parseAsync.

Ok, so that men's we have to make a major version release with this.

DerDrodt commented 3 years ago

ok, so three non-TS expert looking at a TS PR. Hmm...

Yeah, it's not ideal, but also imo not a problem. I can say that there are no errors. There just are some places where it is underspecified. The result of the CSL Exporter, for instance, does currently not provide much help type-wise, because I don't know the exact format. But that can be fixed icrementally.

retorquere commented 3 years ago

The renaming of all files from .js to .ts means that the diffing mechanism doesn't help us very much on those. Not sure if we can somehow avoid that.

That shouldn't really make too much of a difference. Git tracks content, not filenames; renames are inferred if the contents is close enough, although I can't quantify what "close enough" equates to.

Yeah, it's not ideal, but also imo not a problem. I can say that there are no errors.

Agreed on this. My use of TS is pragmatic.

johanneswilm commented 3 years ago

That shouldn't really make too much of a difference. Git tracks content, not filenames; renames are inferred if the contents is close enough, although I can't quantify what "close enough" equates to.

The TS version here adds semicolons to the end of all statements. The JS version does not have them. I don't know if that is the entire reason why git doesn't recognize the files as containing the same content, but it is probably part of the reason.

@DerDrodt I am reading that semicolons are not needed in TS [1]. Is there another reason why they should be added?

[1] https://medium.com/@eugenkiss/dont-use-semicolons-in-typescript-474ccfe4bdb3

DerDrodt commented 3 years ago

@DerDrodt I am reading that semicolons are not needed in TS [1]. Is there another reason why they should be added?

Not as far as I know. There are a few edge cases where it could lead to ambiguities, but I doubt that is the case here. We could try that.

retorquere commented 3 years ago

The TS version here adds semicolons to the end of all statements. The JS version does not have them. I don't know if that is the entire reason why git doesn't recognize the files as containing the same content, but it is probably part of the reason.

100%. Diffing is line-by-line.

@DerDrodt I am reading that semicolons are not needed in TS [1]. Is there another reason why they should be added?

They're not needed in JS either. There are some instances where they are required because they remove ambiguity around what may or may not be a function call (parenthesis can legally be on a separate line in JS/TS), but for the most part they're superfluous; I omit them where possible because they are visual clutter. eslint will tell you whether the semicolon use is needed or not.

johanneswilm commented 3 years ago

OK, so if it is not too much work, could you, @DerDrodt, remove them in your PR? Then hopefully that will lead to a smaller patch.

DerDrodt commented 3 years ago

Hmm, it only helped with some files.

johanneswilm commented 3 years ago

@DerDrodt Ok, but getting closer. I have now run prettier with the same options on the master branch. Could you pull once more?

DerDrodt commented 3 years ago

I will do it tomorrow. Let’s see if that works

retorquere commented 3 years ago

Does travis still work for this repo? Travis has fallen entirely apart for a lot of open source repos. I've migrated everything to GH actions.

johanneswilm commented 3 years ago

I know. I've updated most other packages I maintain to not use travis. We should also update this package, but let's try to do in a separate PR.

On Fri, Apr 30, 2021, 22:31 Emiliano Heyns @.***> wrote:

Does travis still work for this repo? Travis has fallen entirely apart for a lot of open source repos. I've migrated everything to GH actions.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/fiduswriter/biblatex-csl-converter/pull/123#issuecomment-830364060, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAERMOFA72ZD5L5NNG27I6DTLMHRRANCNFSM43XHGNTQ .

johanneswilm commented 3 years ago

@DerDrodt Did you run prettier on all source files? The commands I ran were:

npx prettier --write .
npx eslint src --fix
DerDrodt commented 3 years ago

I also added a format script that does exactly this.

johanneswilm commented 3 years ago

@DerDrodt This already looks a lot better. I manually fixed two more files. if you could merge those as well, I think we should be ready.

DerDrodt commented 3 years ago

Cool. Looks like git finally understood the renames.

johanneswilm commented 3 years ago

@retorquere The patch is now much smaller and it is clearer what is actually changing. I would appreciate it if you could take a quick look, also to make sure we are not overlooking anything about the origdate and crossref fields and that this will not break any other citation management software you can think of.

johanneswilm commented 3 years ago

@DerDrodt This looks good to me. If @retorquere also thinks this is good, I'd be ready to merge it. But I understand you wanted to add more tests/change types before merging, right? At any rate, let me know once you feel it is ready to merge.

retorquere commented 3 years ago

I mean... a ton of the changes are still the stripping of the flow comments, and I'm just not familiar enough with how the parser works to judge the bulk of these changes. The one thing that stands out to me are cosmetics:

johanneswilm commented 3 years ago

@retorquere OK, sounds good. How about origdate and crossref? Some of the test cases that are parsed differently now are the test cases you originally provided, I believe. Does the new output result from those tests look OK to you?

retorquere commented 3 years ago

The origdate & crossref looks good to me -- personally, I'd split off the crossref mapping to a separate json file so that the caller can optionally provide their own mapping (and also because I like larger chunks of data to sit in clearly recognizable data file), but that's maybe a mere difference in aesthetic preference.

johanneswilm commented 3 years ago

@DerDrodt @retorquere Great! Then I think we can say that we see no problems with merging this whenever @DerDrodt feels it is ready. The issues @retorquere mentioned seem like this that can be changed/fixed at a later stage if we decide to do so, but they are not show stoppers and this could basically be released without breaking anyone's existing setup. We do need to make it a major version release though, as there are changes to the API. This should probably also be pointed out in the README.

DerDrodt commented 3 years ago

I just took (part of) @retorquere 's advice and allowed the user to configure the inheritance for crossrefs.

From my point of view, you can merge this.

johanneswilm commented 3 years ago

@DerDrodt Looks good, I will cut a release shortly. Question: Is it important to keep the yarn.lock file in the repo? Trying to run yarn, it doesn't like the presence of the package-lock.json file.

johanneswilm commented 3 years ago

@DerDrodt Also, I see three warnings saying "(!) Plugin typescript: @rollup/plugin-typescript: Rollup 'sourcemap' option must be set to generate source maps.". Is this something you have looked at already?

DerDrodt commented 3 years ago

@DerDrodt Looks good, I will cut a release shortly. Question: Is it important to keep the yarn.lock file in the repo? Trying to run yarn, it doesn't like the presence of the package-lock.json file.

That can be removed. I used yarn, but if you use npm, just remove yarn.lock

DerDrodt commented 3 years ago

@DerDrodt Also, I see three warnings saying "(!) Plugin typescript: @rollup/plugin-typescript: Rollup 'sourcemap' option must be set to generate source maps.". Is this something you have looked at already?

I will look at that when I get back on my computer, but I’m 99% sure that this warning is only for the test builds where I figured source maps are useless. The normal output files should all have sourcemaps.

johanneswilm commented 3 years ago

Version 2.0.0 has been released and it includes this PR. Thanks @DerDrodt for the TS code and also @retorquere for the review!

DerDrodt commented 3 years ago

Cool. I will deprecate my npm package and refer to this one.

Working with you was very cool :)