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

Create 160_Flugans_Stolta_Gång_på_rullgardinen #20

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

Conversation

Chef-mux
Copy link

Fun, classic song

Copy link
Member

@Cactooz Cactooz left a comment

Choose a reason for hiding this comment

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

Looks good.
The by tag does not exist so that can be removed. Also add composer: Otto Lindblad as he is the composer for Längtan till landet.

Then it needs to be build but I can do that if you need help with it :)

@Chef-mux
Copy link
Author

Chef-mux commented Oct 10, 2023

Yes, i would definitely change that, if i knew hoe to ... I'm not completely sure how i made the pull request to begin with...

Edit
I think i found it

Added composer: Otto Lindblad
and removed the By tag
Changed Kräftans to flugans... as that is what it should be
Copy link
Member

@StelFoog StelFoog left a comment

Choose a reason for hiding this comment

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

Great first PR!

A couple of mostly small, and mostly technical, comments:

  • composer needs to be on it's own line, otherwise the parser Is going to believe that the melody is Längtan till landet (Vintern rasat) composer: Otto Lindblad
  • Title and Melody should not be capitalized, the parser only recognizes the specific words title and melody
  • There should be a space between tags: and [snaps, swedish]
  • The name of the file should capitalize words the same way the title does – this doesn't affect anything, but it's good practice to be consistent across all files.
  • When searching around it seems that the far more common version is Kräftans stolta gång på rullgardinen. So unless the Flugan-version is more commonly used within the IN-chapter I think it's preferable to use the more standard version.

Then like @Cactooz has said you also need to "rebuild" the project. Right now it is somewhat complicated, so we're happy to help you out with that when everything's ready ^^

Edit: If you need any help with anything regarding this PR just give me a ping and I'll try my best to be of use

…ans_stolta_sång_på_rullgardinen

@StelFoog 

I have  put composer on it's own line, but i would like it to be noted that it is the composer of the melody, not the writer of theese lyrics.

I have fixed the other issues.

Regarding what is more common, my own searches gave me three common results. Flugan, Kräftan and Loppan. Since i have never heard of anyone besides me in IN-sektionen that knows of this, i would say that flugan is the most commonly used in IN. Also, kräftan is more specific for kräftskiva, whie flugan and loppan are more general versions. One could perhaps include the alternate version for Kräftskiva also?

i will gladly accept help with buidinig
Copy link
Member

@StelFoog StelFoog left a comment

Choose a reason for hiding this comment

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

The name of the file seems to accidentally have become Flugans_stolta_sång_på_rullgardinen instead of gång, I also forgot to mention before that file names should avoid åäö and instead use aao (this is to make it easier to search for songs for people who don't have Swedish keyboards).

Apart from that everything else looks good :)

As regards the composer – composer is specifically to define who wrote the melody. The author of the lyrics is defined by the author.

…gang_pa_rullgardinen

Changed name of file from "sång" to "gång", also removed Å,Ä,Ö from file name
@StelFoog
Copy link
Member

StelFoog commented Oct 26, 2023

Seems like the file is missing the markdown file format (.md) and the tag should be swe instead of swedish.

If you could rebase this (and your other PR) – after the auto-build PR is merged – we would be able to run the tests that do all of this validation for us. I can of course help out with that as well 😄

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