Fork me on GitHub
#clj-yaml
<
2022-09-23
>
grzm14:09:06

Y'all choose. I chose the design I want, understanding the alternatives (even the one you proposed, @borkdude). I've provided my reasoning and believe I've addressed your concerns except for preference. I don't have anything to add. If the consensus of the maintainers is that you want something else, that's the way it should be.

borkdude14:09:54

I think I agree 95% with your design, it's just that: • I think the name could resemble options in similar clojure libraries: unknown-tag-fn -> default-tag-fn • Positional arg functions have been known to be cumbersome to work with, if you want to add stuff to them later. So (fn [tag val] ...) => (fn [{:keys [tag val]}] ...) . Unless we're 100% certain that we will never add something to this, a map is the safe, boring choice.

borkdude14:09:06

Perhaps @lee can have the final say and then I'll un-die on that hill as well.

borkdude14:09:45

I'm just terrified of adding things that break and using maps has been a good way to prevent that, but maybe I'm exaggerating so I'll be ok with @lee having the final word

lread15:09:39

Hiya, I’ll catch up in a bit and then share what I think. Hopefully it won’t be a 3rd opinion! simple_smile

😅 1
borkdude15:09:45

I'd like to do a new bb release today, so I hope we can get this sorted out

👍 1
borkdude15:09:20

(see #babashka-sci-dev)

lread16:09:00

Sorry if any of this has already been sorted… …agree on no question marks on boolean keyword options… maybe more so for consistency… current clj-yaml keywords booleans do not have question marks… ex. mark unsafe

lread16:09:42

..agree that option keywords do not need to be namespaced…

lread16:09:46

…agree that positional args are less extensible/future-proof than a map arg. @grzm are positional args really important to you? Why so strong a position? (pun unintended)

lread16:09:22

…think that unknown-tag-fn is probably more self-describing than default-tag-fn (unless maybe unless you are already familiar with similar clojure libraries?).

borkdude16:09:49

OK, I think we solved it then.

borkdude16:09:44

I hope I didn't piss off @grzm with my feedback...

lread16:09:03

@grzm, this has been a bit of a long slog of a PR! I hope you still love us with all of your heart. ❤️

borkdude16:09:04

He probably has more important stuff to do at StrangeLoop 😞

grzm16:09:56

You're misunderstanding. I fear I'm being misunderstood: I don't have a strong position. I view this as a style thing. If my reasons aren't convincing (I believe they're clear), it's a preference thing, and it's up to the maintainers to decide what they want. I don't want to continue discussion that I don't think is worthwhile.

grzm16:09:44

I've pushed a change which I believe reflects what y'all desire.

borkdude16:09:44

@grzm I'm 100% satisfied now, thank you!

lread16:09:21

@borkdude are you thinking of cutting a clj-yaml release before snakeyaml 1.33? Or are you going to wait? (I don’t think I can really help Andrey with https://bitbucket.org/snakeyaml/snakeyaml/issues/556/update-yamlloadas-signature-to-support)

borkdude16:09:35

I'd like to get the number fix in

👍 1
borkdude16:09:04

but perhaps I should just do multiple bb releases shortly after one another then

borkdude16:09:31

I'll wait a few hours

lread16:09:10

I’ll hold off on https://github.com/clj-commons/clj-yaml/issues/47 until I hear back from @slipset. But I’d like to get back to this task, if just to finish it, then all also apply this work to my https://github.com/clj-commons/clj-http-lite/issues/65.

lread16:09:16

And maybe even also apply it to etaoin and rewrite-clj for clj-commons tag triggered deploy consistency…

slipset17:09:33

So I guess I have some sentimental attachment to Circle. They are/were a Clojure shop, and I’ve been enjoying getting help from and working with @marc-omorain over the years. Where Circle feels like a company, Github and their Actions feels like, eh, Microsoft 🙂

lread17:09:56

No disagreement there!

slipset17:09:58

The main reason we’ve been doing stuff with GA at work is exactly to update the github repo, since it feels weird that Circle should go about pushing stuff to github.

slipset17:09:54

So we have amongst other things this:

name: Release

# Controls when the action will run. Workflow runs when manually triggered using the UI
# or API.
on:
  workflow_dispatch:
    # Inputs the workflow accepts.
    inputs:
      release_type:
        # Friendly description to be shown in the UI instead of 'name'
        description: 'Release type, valid values are major, minor, patch'
        # Default value if no value is explicitly provided
        default: 'patch'
        # Input has to be provided for the workflow to run
        required: true
      skip_tests:
        description: 'Should we skip tests on this release?'
        default: 'false'
        required: false
        
# A workflow run is made up of one or more jobs that can run sequentially or in parallel
jobs:
  # This workflow contains a single job called "greet"
  release:
    # The type of runner that the job will run on
    runs-on: ubuntu-latest

    # Steps represent a sequence of tasks that will be executed as part of the job
    steps:
    - uses: actions/checkout@v2
    - name: setup_git
      run: |
          git config --global user.name '${{ github.actor }}'
          git config --global user.email '${{ github.actor }}@users.noreply.github.com'
    # Runs a single command using the runners shell
    - name: release
      run: ./release.sh ${{ github.event.inputs.release_type }}
    - name: push
      run: git push
at work, which gives us a shiny green button to do deploys from (with some input) from github. Which is nice, since we generally do the squash/merge thing in github.

slipset17:09:36

@vemv’s nvd scanner from GA, since we can run it whenever PRs are changed and stuff

slipset17:09:21

BTW having nvd set up on clj-commons projects would be really nice (and more welcome than switching all stuff to GA 🙂

vemv18:09:11

that would be nice! feel free to @ me for the first few integrations. Please be pedantic (e.g. simply refer to the readme carefully) when setting these up :) often people don't, and end up getting issues (nowadays nvd is a little more proactive against misuse. But still!)

lread17:09:25

Hmmm.. thinking… So what if clj-commons team documents what it prefers for its projects, and let the primary maintainers choose how to implement those? Maybe things like: • keep a changelog • tag releases • release invocable by maintainer from command line • document release flow and how to release • etc. I could help with this.

lread17:09:12

We could move this discussion to #clj-commons if that makes more sense.

👍 1
lread17:09:43

Gotta run for now, but looking forward to thoughts on this!

borkdude18:09:07

@lee @grzm I merged the :unknown-tag-fn PR by resolving the conflicts locally. I needed to port some markdown to adoc in the process 😅

❤️ 1
borkdude18:09:17

Thank you @grzm for your work!

grzm19:09:47

@borkdude Thanks for fixing up the conflicts! ❤️ Grabbing time at strangeloop has been challenging.

borkdude19:09:01

@grzm I figured as much, so I did that boring task for you. Have fun! :)

🙏 1