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

Change publisher to dictionary #105

Merged
merged 9 commits into from
Oct 2, 2024
Merged

Change publisher to dictionary #105

merged 9 commits into from
Oct 2, 2024

Conversation

DerDrodt
Copy link
Contributor

@DerDrodt DerDrodt commented Dec 13, 2023

Follows and fixes #34.

Changes:

  • Changes the publisher field to be either a FormatString or a dictionary with name and location.
  • location field remains but only for events; publisher location must be in the publisher field.
  • Update: Publisher name is now optional

To discuss:

  • Should the location field of an entry be renamed (e.g., event-location) or handled in another way?

TODO:

  • Document changes to format.

@MaxGietl
Copy link
Contributor

MaxGietl commented Dec 13, 2023

Could this be implemented in a way that makes defining a standalone location for everything that isn't an event possible? In my university it seems to be standard practice to sometimes only display the location of a book, not the publisher itself, so I don't always have that information. Right now I have to define an empty string for the publisher, which obviously isn't ideal.
Currently this code is responsible for hiding the location if the publisher is None:

.map(|e| if e.publisher().is_some() { Some(e) } else { None })
but obviously different in this new version.

@DerDrodt
Copy link
Contributor Author

Could this be implemented in a way that makes defining a standalone location for everything that isn't an event possible?

That would kind of be the current implementation. Technically, in my implementation you can always define a location but

  1. It is not intended as far as I am concerned;
  2. Translating from bib(la)tex to hayagriva would ignore the location as you define it.
    Cases like this is why this is a draft: We need some discussion on how the format should be changed.

In my university it seems to be standard practice to sometimes only display the location of a book, not the publisher itself, so I don't always have that information.

If I may ask: Why? And what style are you using? It seems odd and is certainly not intended by most styles.

Right now I have to define an empty string for the publisher, which obviously isn't ideal. Currently this code is responsible for hiding the location if the publisher is None:

.map(|e| if e.publisher().is_some() { Some(e) } else { None })

but obviously different in this new version.

In my version, hayagriva behaves exactly the same: If no publisher is present, it shows no location.

If I am correct, and having a location but no publisher is not intended, adding an empty string seems fair.

@MaxGietl
Copy link
Contributor

MaxGietl commented Dec 14, 2023

This PR might have been the wrong place to bring this up because, as you say, it doesn't change the behaviour I'm talking about. I'm also not sure whether my initial impression is correct that it'd make exposing a location without a publisher more difficult.

I don't know which style they use, I'm not even sure they use a standard because at least one professor sometimes includes the publisher's name, sometimes they don't. I have briefly looked at the citations in two text books and one of them also only prints the location, so my professors don't seem to be that far from the standard of the field (social science). I will ask them though, if I get the chance.

Below are some example citations (in German, unfortunately):
Screenshot_20231214_125609_Drive.jpg

What's the rationale behind hiding the location when the publisher isn't set? My workaround works well, and setting a single empty string isn't too bad, I was just very confused by this behaviour. But documenting that would resolve it as well. Does bibtex do the same? I don't have any experience with that, unfortunately.

@DerDrodt
Copy link
Contributor Author

This PR might have been the wrong place to bring this up because, as you say, it doesn't change the behaviour I'm talking about. I'm also not sure whether my initial impression is correct that it'd make exposing a location without a publisher more difficult.

It’s supposed to make it clear that they belong together, usually.

I don't know which style they use, I'm not even sure they use a standard because at least one professor sometimes includes the publisher's name, sometimes they don't. I have briefly looked at the citations in two text books and one of them also only prints the location, so my professors don't seem to be that far from the standard of the field (social science). I will ask them though, if I get the chance.

That’s very weird. Line I said, it’s supposed to be the location where it’s published, which is odd without a publisher.

Below are some example citations (in German, unfortunately): Screenshot_20231214_125609_Drive.jpg

I’m German so that’s fine. If you can ask someone why they wrote it like that, that would be great!

What's the rationale behind hiding the location when the publisher isn't set? My workaround works well, and setting a single empty string isn't too bad, I was just very confused by this behaviour. But documenting that would resolve it as well. Does bibtex do the same? I don't have any experience with that, unfortunately

BibTeX is too permissive here, it seems.

@MaxGietl
Copy link
Contributor

MaxGietl commented Dec 14, 2023

Okay after looking through some more random sociology and political science books, only two included the publisher's name and five didn't while all included the location. Obviously not representative, but I think it shows that that style is at least somewhat common place.

And don't get me wrong: I think it's pretty weird too and it makes finding the correct book a lot harder but maybe hayagriva might want to support it because enough people seem to use it? Besides, what is the actual gain of forbidding it?

Also I think making the name field optional on the Publisher struct could be enough to support it? That should also make it clear that it's supposed to be the location of the publisher, even if no name is given

@Drodt
Copy link
Contributor

Drodt commented Dec 15, 2023

Okay after looking through some more random sociology and political science books, only two included the publisher's name and five didn't while all included the location. Obviously not representative, but I think it shows that that style is at least somewhat common place.

And don't get me wrong: I think it's pretty weird too and it makes finding the correct book a lot harder but maybe hayagriva might want to support it because enough people seem to use it?

Strange... But yeah, in that case we should support it. The question is whether we consider it rare enough that setting the publisher's name to an empty string is the way to support it (in which case, we should document that), or just disregard this PR.

Besides, what is the actual gain of forbidding it?

The gain of this PR is to structure the information in hayagriva better. And as far as I can see, in the vast majority of styles and for most tools, a location without a publisher is not intended.

Also I think making the name field optional on the Publisher struct could be enough to support it? That should also make it clear that it's supposed to be the location of the publisher, even if no name is given

That's possible. But if someone just writes publisher: some string, should we still consider this the name?

@MaxGietl
Copy link
Contributor

MaxGietl commented Dec 15, 2023

That's possible. But if someone just writes publisher: some string, should we still consider this the name?

I think that's a good compromise, maybe it could accept the following:

guide:
   ...
   publisher: Megadodo

guide:
   ...
   publisher:
      name: Megadodo
      location: Ursa Minor

guide:
   ...
   publisher:
      location: Ursa Minor

Would that work?

@DerDrodt
Copy link
Contributor Author

That looks good

@reknih
Copy link
Member

reknih commented Jan 24, 2024

I think that renaming location to event-location is too broad. What, for example, if you want to encode the Geo-Tag of a social media post or that you are citing the manuscript that your mentor wrote on Base Camp of Mount Everest? Otherwise, this change looks good to me!

@DerDrodt DerDrodt marked this pull request as ready for review October 1, 2024 18:46
Copy link
Contributor

@PgBiel PgBiel left a comment

Choose a reason for hiding this comment

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

This looks nice! Here are some observations (paging @reknih for further thoughts).

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/types/mod.rs Show resolved Hide resolved
Drodt and others added 4 commits October 2, 2024 06:38
Co-authored-by: PgBiel <9021226+PgBiel@users.noreply.github.com>
@reknih reknih merged commit c42aaa5 into typst:main Oct 2, 2024
2 checks passed
@reknih
Copy link
Member

reknih commented Oct 2, 2024

Thank you!

@PgBiel PgBiel mentioned this pull request Oct 2, 2024
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.

publisher should be an object
5 participants