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

[6574] Added support for biblatex-software #6747

Merged
merged 3 commits into from
Aug 22, 2020

Conversation

hemantgs
Copy link
Contributor

@hemantgs hemantgs commented Aug 9, 2020

Added change to changelog
Fixes #6574

This PR is in reference to #6574
Added the new biblatex software entries under a new divider like the below image

biblatex-software

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@hemantgs
Copy link
Contributor Author

hemantgs commented Aug 9, 2020

Hi @koppor
I have gone ahead and made a draft set of changes for #6747 ,

  • Created a divider like in IEEETran for BibTex and added the software entries under it
    I am yet a little un-clear on the Customized Entry type flow , so I have not progressed on that , do give your thoughts

@calixtus
Copy link
Member

calixtus commented Aug 9, 2020

Hi @hemantgs ,
on first sight codewise your changes look good to me.
Thank you very much for your work on this feature.

I also was thinking about abstraction of this feature in the mid to far future, maybe something to decide on JabCon2020, so don't think of it in this PR: One could activate native support for biblatex-software or IEEE or for music or other biblatex drivers as an additional feature in the preferences.

@Siedlerchr
Copy link
Member

Yep codewise looks already good!

@hemantgs hemantgs marked this pull request as ready for review August 9, 2020 17:35
@hemantgs
Copy link
Contributor Author

hemantgs commented Aug 9, 2020

@calixtus @Siedlerchr Cool 👍
Would it make sense to add change to the documentation page also ?

@Siedlerchr
Copy link
Member

Yes, I think that makes sense and to have a nice newer screenshot for here and the entry id manually
https://docs.jabref.org/collect/add-entry-manually
And it would then be nice if you could add a hint that the New Entry types dialogue content is dependent on the bibdatabase mode
https://docs.jabref.org/collect/add-entry-using-an-id

Added change to changelog

 md checkstyle fixed

checkstyle fixed
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 18, 2020
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Thank you for working on it. I have some refinemenets to the adr.


## Context and Problem Statement

JabRef does not right now have support for Biblatex-Software out of the box , users have to add custome entry type.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
JabRef does not right now have support for Biblatex-Software out of the box , users have to add custome entry type.
JabRef does not right now have support for Biblatex-Software out of the box, users have to add custome entry type.

## Context and Problem Statement

JabRef does not right now have support for Biblatex-Software out of the box , users have to add custome entry type.
With citing software becoming fairly comen , native support would be helpful.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
With citing software becoming fairly comen , native support would be helpful.
With citing software becoming fairly common, native support is helpful.


## Decision Drivers

* The new entry types definitions should be added to the Select Entry Pane and be separated by a divider
Copy link
Member

Choose a reason for hiding this comment

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

I think, this line should be removed. It is not a "global driver", but one implementation option.

The other point is OK. - Even if it is a single one.


## Considered Options

* Adding the new entry types to the existing biblatex types , but it conflicted with an already existing type(software)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Adding the new entry types to the existing biblatex types , but it conflicted with an already existing type(software)
* Add the new entry types to the existing biblatex types


### Adding the new entry types to the existing biblatex types

* Good, since ther is no need for a new category in the add entry pane
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Good, since ther is no need for a new category in the add entry pane
* Good, because there is no need for a new category in the add entry pane
* Bad, because it conflicts with an already existing type (software)

note = {First Scilab version. It was distributed by anonymous ftp.},
crossref = {delebecque:hal-02090402}
}
@software {cgal,
Copy link
Member

Choose a reason for hiding this comment

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

Please add an empty line before

introducedin = {cgal:3-1},
url = {https://doc.cgal.org/5.0.2/Manual/packages.html#PkgVoronoiDiagram2},
}
@softwaremodule{cgal:lp-gi-20a-condensed,
Copy link
Member

Choose a reason for hiding this comment

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

Please add an empty line before

origin=https://github.com/CGAL/cgal/},
url = {https://doc.cgal.org/5.0.2/Manual/packages.html#PkgVoronoiDiagram2},
}
@software {parmap,
Copy link
Member

Choose a reason for hiding this comment

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

Please add an empty line before

## Considered Options

* Adding the new entry types to the existing biblatex types , but it conflicted with an already existing type(software)
* Add a divider with label Biblatex-Software underwhich the new entries are listed : Native support for Biblatex-Software
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Add a divider with label Biblatex-Software underwhich the new entries are listed : Native support for Biblatex-Software
* Add a divider with label Biblatex-Software under which the new entries are listed: Native support for Biblatex-Software


## Decision Outcome

Chosen option: Yet to be decided.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Chosen option: Yet to be decided.
Chosen option: Add a new devider, because comes out best (see below).

@hemantgs
Copy link
Contributor Author

Thank you for working on it. I have some refinemenets to the adr.

Hi @koppor I have updated the ADR per the review comments , please do have a look

@hemantgs hemantgs requested a review from koppor August 19, 2020 14:09
@Siedlerchr Siedlerchr merged commit 9d17101 into JabRef:master Aug 22, 2020
@Siedlerchr
Copy link
Member

Thank you very much for your contribution and for creating an ADR!

Siedlerchr added a commit that referenced this pull request Aug 22, 2020
* upstream/master:
  Streamline new library command (#6773)
  [6574] Added support for biblatex-software (#6747)
  Updated the eclipse doc with a note and image on how to add the run/debug configuration. (#6776)
  New Crowdin updates (#6774)
Siedlerchr added a commit that referenced this pull request Aug 22, 2020
* upstream/master:
  Streamline new library command (#6773)
  [6574] Added support for biblatex-software (#6747)
  Updated the eclipse doc with a note and image on how to add the run/debug configuration. (#6776)
  New Crowdin updates (#6774)
@koppor
Copy link
Member

koppor commented Aug 23, 2020

Id id not the final check on the PR. We missed two things now breaking master:

Unit test related to this PR fail

grafik

markdown-lint

This is an easy fix. (Linter ensure consistent formatting of all files)

grafik

@koppor
Copy link
Member

koppor commented Aug 23, 2020

markdown-lint is already fixed in master 🎉

@koppor
Copy link
Member

koppor commented Aug 23, 2020

Not sure why the other PR complained ther: https://github.com/JabRef/jabref/runs/1016112813?check_suite_focus=true

@Siedlerchr
Copy link
Member

Markdown was fixed. I overlooked the unit test probably because of the failing architecture thing.
Probably need to add those b iblatex software somehow to the test. Can look into this tomorrow

@Siedlerchr
Copy link
Member

Fixed by @calixtus

@koppor
Copy link
Member

koppor commented Aug 23, 2020

Test fixed at #6780.

@hemantgs
Copy link
Contributor Author

Looks like I overlooked the unit test , apologies guys

@calixtus
Copy link
Member

We did so too, no need to apologize. 😉

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.

Add support for biblatex-software
4 participants