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

Remove remaining directory server mentions #3017

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Feb 25, 2023

Short description of changes

The overall goal is that:

  • Directory is the Jamulus term for an application that supplies a list of Jamulus Servers on request (most often the Jamulus application running as --server --directoryaddress localhost (or -s -e 127.0.0.1)).
  • Directory Address is the IP address (IPv4 only, currently), possibly including port, used to contact the Directory.
  • "Directory server" is an invalid term in Jamulus vocabulary.

This removes remaining "directory server" mentions in the code base (based on grep -ni directory\ \*server src/*.*).

Apart from:

  • backwards compatibility for --directoryserver command line option
  • RPC doccomment

(Replaced: #2997 - this is more complete)

CHANGELOG: Server: Rename --directoryserver to --directoryaddress (and internal changes)

Context: Fixes an issue?

On going maintenance to avoid confusion in terminology.

Does this change need documentation? What needs to be documented and how?

Yes. --help output now says --directoryaddress plus a few other changes. Translations affected in places, too (I added some more server GUI ones).

Status of this Pull Request

It runs okay.

What is missing until this pull request can be merged?

  • Review.
  • Fixing generate_json_rpc_docs.py script (@hoffie ?)
  • Fixing clang_format...

Note: the Json RPC "directory_server" mention is in its external protocol, not just a comment, so needs consideration before just changing it. The other changes here affect nothing outside Jamulus.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@pljones
Copy link
Collaborator Author

pljones commented Feb 25, 2023

Huh?
image
(Raised #3018)

And Github clang_format and Ubuntu seem to have got out of alignment on my machine again..!

@pljones
Copy link
Collaborator Author

pljones commented Feb 25, 2023

So

"--clang-format-executable" "/clang-format/clang-format10"
$ clang-format --version
Ubuntu clang-format version 14.0.0-1ubuntu1

I guess I need to find clang-format-10 again...

@pljones
Copy link
Collaborator Author

pljones commented Feb 25, 2023

Well, so clang-format-10 is no longer available and installing 11 made no difference locally.

$ sudo apt install clang-format<TAB><TAB>
clang-format     clang-format-11  clang-format-12  clang-format-13  clang-format-14  clang-format-15

(Raised #3019)

src/serverrpc.cpp Outdated Show resolved Hide resolved
@ann0see
Copy link
Member

ann0see commented Feb 25, 2023

Would --directory instead of --directoryaddress be confusing?

@pljones
Copy link
Collaborator Author

pljones commented Feb 25, 2023

Would --directory instead of --directoryaddress be confusing?

I had --directory until this afternoon. But it's the address (in Internet Address including Port terms) of the Directory, not the Directory itself. See "The overall goal is that:" in the PR description.

@pljones pljones force-pushed the bugfix/remove-remaining-directory-server-mentions branch 2 times, most recently from 592a085 to f047a8b Compare February 25, 2023 18:37
src/util.h Outdated Show resolved Hide resolved
src/settings.cpp Outdated Show resolved Hide resolved
@pljones pljones force-pushed the bugfix/remove-remaining-directory-server-mentions branch from f047a8b to 80452df Compare March 26, 2023 15:16
pljones added a commit to jamulussoftware/jamuluswebsite that referenced this pull request Mar 26, 2023
@pljones pljones force-pushed the bugfix/remove-remaining-directory-server-mentions branch 2 times, most recently from 7394039 to f6b99a4 Compare March 28, 2023 20:28
@pljones
Copy link
Collaborator Author

pljones commented Apr 4, 2023

OK, I don't think there's anything outstanding here now?

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

See comment.

src/serverlist.cpp Show resolved Hide resolved
@pljones pljones mentioned this pull request Apr 5, 2023
5 tasks
@pljones pljones force-pushed the bugfix/remove-remaining-directory-server-mentions branch from f6b99a4 to 8f09aff Compare April 6, 2023 17:42
@pljones pljones self-assigned this Apr 9, 2023
@pljones pljones added refactoring Non-behavioural changes, Code cleanup needs documentation PRs requiring documentation changes or additions labels Apr 9, 2023
@pljones pljones added this to the Release 3.10.0 milestone Apr 9, 2023
src/settings.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
Copy link
Member

@ann0see ann0see 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 by sight reviewing this looks ok.

@pljones pljones force-pushed the bugfix/remove-remaining-directory-server-mentions branch from 8f09aff to b1f9602 Compare April 13, 2023 17:22
@pljones pljones merged commit b6314d0 into jamulussoftware:main Apr 16, 2023
@pljones pljones deleted the bugfix/remove-remaining-directory-server-mentions branch April 16, 2023 15:09
@pljones
Copy link
Collaborator Author

pljones commented Apr 17, 2023

Raised jamulussoftware/jamuluswebsite#919 for the documentation change.

@pljones
Copy link
Collaborator Author

pljones commented Apr 18, 2023

https://github.com/jamulussoftware/jamulus/actions/runs/4713980244/jobs/8379530170 is still failing on main (but didn't on the branch, which was up to date with main...). Anyone understand why?

Raised #3049

@ann0see
Copy link
Member

ann0see commented Jul 24, 2023

@pljones I think we can drop the needs documentation flag here too.

@pljones pljones removed the needs documentation PRs requiring documentation changes or additions label Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Non-behavioural changes, Code cleanup
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants