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

PB-1083: Add simple versioning #91

Merged
merged 3 commits into from
Nov 8, 2024

Conversation

sami-nouidri-swisstopo
Copy link
Contributor

Issue :

The old folder structure & naming scheme lacked simple versioning in its denomination. As such, it needs to be changed to reflect the version number. Legacy icons in /static/images/babs are not affected.

Fix:

The folder structure has been renamed, and all references to existing babs-{LANG} icon sets have been changed to reflect the new naming scheme.

Copy link

@schtibe schtibe left a comment

Choose a reason for hiding this comment

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

I think the code is OK apart from the changes in logging-cfg-local.yml. Maybe look into that again if you really need these.

Now for the commit messages: I thought we had a guideline on those, but I can't find them. Maybe @ltshb knows.

Usually we put the ticket number in front. Also, I think it's common to use the imperative, see this: https://cbea.ms/git-commit/#imperative

@@ -1,13 +1,13 @@
version: 1
disable_existing_loggers: False # this allow to get logger at module level
disable_existing_loggers: false # this allow to get logger at module level
Copy link

Choose a reason for hiding this comment

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

Interesting. I think this was upper case because this was read by python? You shouldn't be needing ansible-lint here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was checking that : the default for yaml boolean capitalization for yaml linter is all lowercase. I would be in favor of keeping it as Sami did, since it would bring the config file to a standard value.

With that being said, it would be best to also do so in the base template and be consistent across all repos, but those are small changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that makes sense. I just ran lint on the whole project, I'm assuming then that you only run it on code files?

Copy link
Contributor

@ltkum ltkum 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 to me :)

@ltkum
Copy link
Contributor

ltkum commented Nov 8, 2024

Now for the commit messages: I thought we had a guideline on those, but I can't find them. Maybe @ltshb knows.

The commit messages should be :

{TICKET_ID}: summary of the changes
- change 1
- change 2

(if the summary is explicit by itself, no need for a list of changes).

@sami-nouidri-swisstopo
Copy link
Contributor Author

Now for the commit messages: I thought we had a guideline on those, but I can't find them. Maybe @ltshb knows.

The commit messages should be :

{TICKET_ID}: summary of the changes
- change 1
- change 2

(if the summary is explicit by itself, no need for a list of changes).

I'll keep that in mind, I don't think I can change commit messages now 😄

…following PR review. -renamed all occurrences of true/false to True/False
@sami-nouidri-swisstopo sami-nouidri-swisstopo merged commit 416a337 into develop Nov 8, 2024
3 checks passed
@sami-nouidri-swisstopo sami-nouidri-swisstopo deleted the feat-PB-1083-add-simple-versioning branch November 8, 2024 14:46
@ltshb
Copy link
Contributor

ltshb commented Nov 11, 2024

Now for the commit messages: I thought we had a guideline on those, but I can't find them. Maybe @ltshb knows.

The commit messages should be :

{TICKET_ID}: summary of the changes
- change 1
- change 2

(if the summary is explicit by itself, no need for a list of changes).

@ltkum @schtibe @sami-nouidri-swisstopo the commit message from @ltkum is not 100% correct:

PB-XXX: Summary of the changes (should be less than 100 character and one line)
BLANK LINE !
What is the problem that the commit try to solve

How and why you solve the problem this way

Listing changes that can be easily seen in code diff in commit message don't help, it help more to add description on the why and how to understand the changes.

See https://github.com/geoadmin/doc-guidelines/blob/master/GIT_FLOW.md#pull-request-rules

@sami-nouidri-swisstopo
Copy link
Contributor Author

Now for the commit messages: I thought we had a guideline on those, but I can't find them. Maybe @ltshb knows.

The commit messages should be :

{TICKET_ID}: summary of the changes
- change 1
- change 2

(if the summary is explicit by itself, no need for a list of changes).

@ltkum @schtibe @sami-nouidri-swisstopo the commit message from @ltkum is not 100% correct:

PB-XXX: Summary of the changes (should be less than 100 character and one line)
BLANK LINE !
What is the problem that the commit try to solve

How and why you solve the problem this way

Listing changes that can be easily seen in code diff in commit message don't help, it help more to add description on the why and how to understand the changes.

See https://github.com/geoadmin/doc-guidelines/blob/master/GIT_FLOW.md#pull-request-rules

Good to know, thanks. I should look to making my future commits with VS Code, as writing verbose messages is not easy in command line

@schtibe
Copy link

schtibe commented Nov 11, 2024

For those doing it with (n)vim: if the configuration is correct, it shows if there are problems:
image

image

@sami-nouidri-swisstopo sami-nouidri-swisstopo restored the feat-PB-1083-add-simple-versioning branch November 13, 2024 16:23
@sami-nouidri-swisstopo sami-nouidri-swisstopo deleted the feat-PB-1083-add-simple-versioning branch November 13, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants